From owner-freebsd-bugs Mon Mar 25 19: 0:13 2002 Delivered-To: freebsd-bugs@hub.freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.org [216.136.204.21]) by hub.freebsd.org (Postfix) with ESMTP id 0660637B400 for ; Mon, 25 Mar 2002 19:00:04 -0800 (PST) Received: (from gnats@localhost) by freefall.freebsd.org (8.11.6/8.11.6) id g2Q304J94344; Mon, 25 Mar 2002 19:00:04 -0800 (PST) (envelope-from gnats) Date: Mon, 25 Mar 2002 19:00:04 -0800 (PST) Message-Id: <200203260300.g2Q304J94344@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org Cc: From: Bruce Evans Subject: Re: kern/36309: [patch] Wrong mnt_iosize_max calculation in FFS Reply-To: Bruce Evans Sender: owner-freebsd-bugs@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org The following reply was made to PR kern/36309; it has been noted by GNATS. From: Bruce Evans To: Thomas Quinot Cc: FreeBSD-gnats-submit@FreeBSD.ORG Subject: Re: kern/36309: [patch] Wrong mnt_iosize_max calculation in FFS Date: Tue, 26 Mar 2002 13:55:03 +1100 (EST) 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 To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-bugs" in the body of the message