From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay2.corp.sgi.com [137.38.102.29]) by oss.sgi.com (Postfix) with ESMTP id C218D7F3F for ; Tue, 15 Apr 2014 17:06:06 -0500 (CDT) Received: from cuda.sgi.com (cuda2.sgi.com [192.48.176.25]) by relay2.corp.sgi.com (Postfix) with ESMTP id AFDFE30404E for ; Tue, 15 Apr 2014 15:06:06 -0700 (PDT) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by cuda.sgi.com with ESMTP id 5TtwR8PpAKMPVqzQ for ; Tue, 15 Apr 2014 15:06:05 -0700 (PDT) Date: Tue, 15 Apr 2014 18:06:00 -0400 From: Brian Foster Subject: Re: [PATCH 3/9] repair: ensure prefetched buffers have CRCs validated Message-ID: <20140415220559.GE3470@laptop.bfoster> References: <1397550301-31883-1-git-send-email-david@fromorbit.com> <1397550301-31883-4-git-send-email-david@fromorbit.com> <20140415194000.GB3470@laptop.bfoster> <20140415214642.GN15995@dastard> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20140415214642.GN15995@dastard> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Dave Chinner Cc: xfs@oss.sgi.com On Wed, Apr 16, 2014 at 07:46:42AM +1000, Dave Chinner wrote: > On Tue, Apr 15, 2014 at 03:40:00PM -0400, Brian Foster wrote: > > On Tue, Apr 15, 2014 at 06:24:55PM +1000, Dave Chinner wrote: > > > From: Dave Chinner > > > > > > Prefetch currently does not do CRC validation when the IO completes > > > due to the optimisation it performs and the fact that it does not > > > know what the type of metadata into the buffer is supposed to be. > > > Hence, mark all prefetched buffers as "suspect" so that when the > > > end user tries to read it with a supplied validation function the > > > validation is run even though the buffer was already in the cache. > > > > > > Signed-off-by: Dave Chinner > > > --- > > > include/libxfs.h | 1 + > > > libxfs/rdwr.c | 36 +++++++++++++++++++++++++++++++----- > > > repair/prefetch.c | 3 +++ > > > 3 files changed, 35 insertions(+), 5 deletions(-) > > > > > > diff --git a/include/libxfs.h b/include/libxfs.h > > > index 6bc6c94..6b1e276 100644 > > > --- a/include/libxfs.h > > > +++ b/include/libxfs.h > > > @@ -333,6 +333,7 @@ enum xfs_buf_flags_t { /* b_flags bits */ > > > LIBXFS_B_STALE = 0x0004, /* buffer marked as invalid */ > > > LIBXFS_B_UPTODATE = 0x0008, /* buffer is sync'd to disk */ > > > LIBXFS_B_DISCONTIG = 0x0010, /* discontiguous buffer */ > > > + LIBXFS_B_UNCHECKED = 0x0020, /* needs verification */ > > > > This is used in the first couple patches, so it should probably be > > defined earlier (or shuffle those patches appropriately). > > Ah, I busted that on shuffling the patchset, and hadn't done a > patch-by-patch compile. Well spotted! > > > > > > }; > > > > > > #define XFS_BUF_DADDR_NULL ((xfs_daddr_t) (-1LL)) > > > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c > > > index 7208a2f..a8f06aa 100644 > > > --- a/libxfs/rdwr.c > > > +++ b/libxfs/rdwr.c > > > @@ -718,12 +718,25 @@ libxfs_readbuf(struct xfs_buftarg *btp, xfs_daddr_t blkno, int len, int flags, > > > bp = libxfs_getbuf(btp, blkno, len); > > > if (!bp) > > > return NULL; > > > - if ((bp->b_flags & (LIBXFS_B_UPTODATE|LIBXFS_B_DIRTY))) > > > + > > > + /* > > > + * if the buffer was prefetched, it is likely that it was not > > > + * validated. Hence if we are supplied an ops function and the > > > + * buffer is marked as unchecked, we need to validate it now. > > > + */ > > > + if ((bp->b_flags & (LIBXFS_B_UPTODATE|LIBXFS_B_DIRTY))) { > > > + if (ops && (bp->b_flags & LIBXFS_B_UNCHECKED)) { > > > + bp->b_error = 0; > > > + bp->b_ops = ops; > > > + bp->b_ops->verify_read(bp); > > > + bp->b_flags &= ~LIBXFS_B_UNCHECKED; > > > > Should we always expect an unchecked buffer to be read with an ops > > vector before being written? Even if so, this might look cleaner if we > > didn't encode the possibility of running a read verifier on a dirty > > buffer. I presume that would always fail as the crc is updated in the > > write verifier. > > It should fail, and that's a good thing because writing to an > unchecked buffer would indicate that we didn't validate it properly > in the first place. Hence I thought that doing it this way leaves > a canary that traps other problem usage with unchecked buffers. > > Realistically, we shouldn't be writing unchecked buffers - prefetch > doesn't touch buffers, it just does IO, and so someone else has to > read the buffers before they can be dirtied. If it's read without an > ops structure then modified and read again with an ops structure, > we'll catch it... > Ah, I see. That sounds good, but a small comment there with the reasoning to allow a read verifier to run on a dirty buffer would be nice. :) Brian > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs