On Fri, Jan 08, 2016 at 10:55:58AM +1100, Dave Chinner wrote:
> [slightly hacked up quoting order so it makes sense]
>
> On Thu, Jan 07, 2016 at 07:44:33AM -0500, Brian Foster wrote:
> > On Thu, Jan 07, 2016 at 02:08:30PM +1100, Dave Chinner wrote:
> > >
> > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > >
> > > When we do dquot readahead in log recovery, we do not use a verifier
> ......
> > > write operation as well as a read operation that marks the buffer as
> > > not done if any corruption is detected. Also make sure we don't run
>
> Marking the buffer not done mentioned here...
>
> .....
> > >
> > > /*
> > > + * readahead errors are silent and simply leave the buffer as !done so a real
> > > + * read will then be run with the xfs_dquot_buf_ops verifier. See
> > > + * xfs_inode_buf_verify() for why we use EIO and ~XBF_DONE here rather than
> > > + * reporting the failure.
>
> ... and here ...
>
> > > @@ -62,11 +62,14 @@ xfs_inobp_check(
> > > * has not had the inode cores stamped into it. Hence for readahead, the buffer
> > > * may be potentially invalid.
> > > *
> > > - * If the readahead buffer is invalid, we don't want to mark it with an error,
> > > - * but we do want to clear the DONE status of the buffer so that a followup read
> > > - * will re-read it from disk. This will ensure that we don't get an unnecessary
> > > - * warnings during log recovery and we don't get unnecssary panics on debug
> > > - * kernels.
> > > + * If the readahead buffer is invalid, we need to mark it with an error and
> > > + * clear the DONE status of the buffer so that a followup read will re-read it
> > > + * from disk. We don't report the error otherwise to avoid warnings during log
>
> ... and here ...
>
> > Do we really need to clear the flag if the I/O infrastructure doesn't
> > set it until after the verifier is invoked? It's harmless, [...]
> > ... but it does look slightly confusing, without a mention in the
> > comments at least.
>
> I mentioned it 3 times. ;)
>
> Is there something I can do to make it more obvious what the
> comments are referring to?
>
It's obvious from the code alone that the verifier marks the buffer not
done. ;) I'm not not sure how that relates to my question... which is
why does the verifier clear the flag before we've reached the point
where the flag would ever be set (when setting the error state is
sufficient)?
> > > + */
> > > +static void
> > > +xfs_dquot_buf_readahead_verify(
> > > + struct xfs_buf *bp)
> > > +{
> > > + struct xfs_mount *mp = bp->b_target->bt_mount;
> > > +
> > > + if (!xfs_dquot_buf_verify_crc(mp, bp) ||
> > > + !xfs_dquot_buf_verify(mp, bp, 0)) {
> > > + xfs_buf_ioerror(bp, -EIO);
> > > + bp->b_flags &= ~XBF_DONE;
> >
> > Do we really need to clear the flag if the I/O infrastructure doesn't
> > set it until after the verifier is invoked? It's harmless, so if the
> > intent is to just be cautious or future-proof:
>
> It's intended to cover the case that a verifier is run on a buffer
> that has already been read in successfully. This doesn't occur in
> the kernel code, but it most definitely does in the userspace code.
> Strictly speaking it's not necessary (at the moment) for the
> userspace code as it ignores the kernel buffer flags, but I figured
> it's better to leave it there for documentation of expected
> behaviour and to ensure we don't leave a landmine if we ever run a
> readahead verifier on a buffer that is already marked as done...
>
Ok, that's kind of what I figured modulo the userspace case. Sounds
reasonable enough, thanks.
Brian
> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
>
> Thanks!
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
