Date: Mon, 25 Mar 2002 19:00:04 -0800 (PST) From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/36309: [patch] Wrong mnt_iosize_max calculation in FFS Message-ID: <200203260300.g2Q304J94344@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/36309; it has been noted by GNATS.
From: Bruce Evans <bde@zeta.org.au>
To: Thomas Quinot <thomas@cuivre.fr.eu.org>
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200203260300.g2Q304J94344>