From owner-freebsd-bugs Tue Feb 12 5:54:32 2002 Delivered-To: freebsd-bugs@freebsd.org Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by hub.freebsd.org (Postfix) with ESMTP id F31C637B400; Tue, 12 Feb 2002 05:53:46 -0800 (PST) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id XAA26950; Tue, 12 Feb 2002 23:54:21 +1100 Date: Tue, 12 Feb 2002 23:57:15 +1100 (EST) From: Bruce Evans X-X-Sender: To: Matthew Dillon Cc: Matteo , , , Subject: Re: Crash System with MSDOS file-system driver! In-Reply-To: <200202112006.g1BK6Dw56980@apollo.backplane.com> Message-ID: <20020212225510.F4096-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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 On Mon, 11 Feb 2002, Matthew Dillon wrote: > Well, I think you did find a bug in VFS/BIO. Even UFS will unnecessarily > break if a fully valid buffer is written and the write fails, due to > b_resid being left non-zero (B_ERROR is cleared in that case and the > write is re-queued). I think that failure is fairly safe. The read will just terminate early for the bad block. > But we have a problem here. You can't gratuitously clear b_resid for > the B_CACHE case. NFS depends on it being persistent for VLNK handling. > In fact, NFS even assumes it is persistent for directory reading but > since VMIO-backed buffers can be dropped and later reconstituted (zeroing > b_resid), there is a hack in there to magically fix it for the VDIR > case. I forgot about nfs. > I also don't think it's a good idea to just munge breadn() and leave > bread() alone. bread() just calls breadn(). > What I recommend is that the b_resid clearing code actually be put in > getblk() itself but only for the B_CACHE|B_VMIO case, and with a big > comment describing why it is there. If you want to work up a patch > for that, Bruce, I'll be happy to help test it. Or I can just do it > if you don't want to. The code is almost exactly the same. Please do it. > I do not understand the last bit of your patch that converts a non-error'd > short read into an EIO. This case cannot happen on the first bp with > either UFS or MSDOSFS because they explicitly conditionalize the breadn() > call for the case where there is at least one full buffer's worth of data > is left in the file or directory. Consequently the first buffer will > never have a short read. It is just a sanity check. Device drivers return a short read for EOF at least. The EOF case shouldn't happen because filesystems are built of whole blocks. Except I forgot the nfs case... I knew that short reads were non-errors for block devices and thought that removing block devices made them errors in all cases. > The API, in general, does not explicitly > disallow EOF occuring in the first buffer so unless you believe an API > change is in order and clearly document it in breadn()'s comments, you > should not commit this patch. (For example, NFS doesn't use breadn() > but if it did it would likely assume that EOF could occur anywhere, even > in the first buffer). It's not a good API, so some changes to it wouldn't hurt. Currently, callers of bread() are apparently required to detect short reads by comparing bp->b_bcount - bp->b_resid with their requested size. Most callers don't bother. physio() is the main exception. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-bugs" in the body of the message