When mounting an FFS file system, the mnt_iosize_max attribute of the mount point is supposed to be set to a value no greater than the si_iosize_max of the underlying device, but the comparison between the two values is made in the wrong direction. The consequence is that, in some circumstances, IO requests to a device may be made with a size that exceeds its si_iosize_max. Fix: The patch below seems to fix the problem for me.
On Mon, 25 Mar 2002, Thomas Quinot wrote: > >Description: > When mounting an FFS file system, the mnt_iosize_max > attribute of the mount point is supposed to be set to > a value no greater than the si_iosize_max of the underlying > device, but the comparison between the two values is > made in the wrong direction. Actually, it is supposed to increase the default value of mnt_iosize_max (which I think is always DFLTPHYS) to the size actually supported by the device. The comparsion is in the correct direction for that. This is used mainly by ata devices to increase the size from 64K (to 128K for ata disks). > The consequence is that, in some circumstances, IO requests > to a device may be made with a size that exceeds its > si_iosize_max. Yes, the case where si_iosize_max is less than DFLTPHYS is broken. I have used the following fix in ffs and ext2fs for so long that I had forgotten about it: %%% Index: ffs_vfsops.c =================================================================== RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_vfsops.c,v retrieving revision 1.172 diff -u -2 -r1.172 ffs_vfsops.c --- ffs_vfsops.c 19 Mar 2002 22:40:47 -0000 1.172 +++ ffs_vfsops.c 24 Mar 2002 10:24:47 -0000 @@ -604,13 +589,10 @@ if (error) return (error); - if (devvp->v_rdev->si_iosize_max > mp->mnt_iosize_max) + if (devvp->v_rdev->si_iosize_max != 0) mp->mnt_iosize_max = devvp->v_rdev->si_iosize_max; +#ifdef bloat if (mp->mnt_iosize_max > MAXPHYS) mp->mnt_iosize_max = MAXPHYS; - - if (VOP_IOCTL(devvp, DIOCGPART, (caddr_t)&dpart, FREAD, cred, td) != 0) - size = DEV_BSIZE; - else - size = dpart.disklab->d_secsize; +#endif bp = NULL; %%% The idea here is that the device driver should always know the correct size, so the code should be simply: mp->mnt_iosize_max = devvp->v_rdev->si_iosize_max here; however, there may be broken drivers, so we set mp->mnt_iosize_max to a default value (DFLTPHYS) elsewhere and keep using that value if devvp->v_rdev->si_iosize_max is 0 (which indicates a broken driver). OTOH, there are no known drivers or default setters that are so broken as to set an iosize to > MAXPHYS, so we don't need to check that the result is <= MAXPHYS here; that check belongs in the drivers. So I put it in "#ifdef bloat". The DIOCGPART change is to remove (part of) unrelated bloat/garbage. I think mp->mnt_iosize_max is only used in vfs_cluster.c, so it only needs to be set in filesystems that use vfs_cluster.c. This seems to be broken in cd9660, so acd's setting of si_iosize_max to 252 * DEV_BSIZE is never used, and there might be i/o errors for cd device drivers that set si_iosize_max to < DFLTPHYS. Bruce
Le 2002-03-26, Bruce Evans écrivait : > Actually, it is supposed to increase the default value of mnt_iosize_max > (which I think is always DFLTPHYS) to the size actually supported by the > device. The comparsion is in the correct direction for that. This is > used mainly by ata devices to increase the size from 64K (to 128K for > ata disks). OK, makes sense. > I think mp->mnt_iosize_max is only used in vfs_cluster.c, so it only > needs to be set in filesystems that use vfs_cluster.c. Yep. As a matter of fact, I stumbled on that problem with a device that required an si_iosize_max of 32 Kb. Once that was set in the driver, I could newfs it just fine, but copying files onto it would still produce errors. Do you think you can commit your fix? Thanks, Thomas. -- Thomas.Quinot@Cuivre.FR.EU.ORG
State Changed From-To: open->closed Fixed in ffs_vfsops 1.173 in -current. Fixed in ffs_vfsops 1.117.2.9 in RELENG_4. Similarly in cd9660 and ext2fs.