On Wed, Jan 06, 2016 at 03:00:34PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> When we do dquot readahead in log recovery, we do not use a verifier
> as the underlying buffer may not have dquots in it. e.g. the
> allocation operation hasn't yet been replayed. Hence we do not want
> to fail recovery because we detect an operation to be replayed has
> not been run yet. This problem was addressed for inodes in commit
> d891400 ("xfs: inode buffers may not be valid during recovery
> readahead") but the problem was not recognised to exist for dquots
> and their buffers as the dquot readahead did not have a verifier.
>
> The result of not using a verifier is that when the buffer is then
> next read to replay a dquot modification, the dquot buffer verifier
> will only be attached to the buffer if *readahead is not complete*.
> Hence we can read the buffer, replay the dquot changes and then add
> it to the delwri submission list without it having a verifier
> attached to it. This then generates warnings in xfs_buf_ioapply(),
> which catches and warns about this case.
>
> Fix this and make it handle the same readahead verifier error cases
> as for inode buffers by adding a new readahead verifier that has a
> 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
> readahead if the dquot buffer has been marked as cancelled by
> recovery.
>
> This will result in readahead either succeeding and the buffer
> having a valid write verifier, or readahead failing and the buffer
> state requiring the subsequent read to resubmit the IO with the new
> verifier. In either case, this will result in the buffer always
> ending up with a valid write verifier on it.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_dquot_buf.c | 32 ++++++++++++++++++++++++++------
> fs/xfs/libxfs/xfs_quota_defs.h | 2 +-
> fs/xfs/libxfs/xfs_shared.h | 1 +
> fs/xfs/xfs_log_recover.c | 9 +++++++--
> 4 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index 11cefb2..936952d 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
...
> @@ -264,6 +264,21 @@ xfs_dquot_buf_read_verify(
> }
>
> /*
> + * 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.
> + */
> +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))
> + bp->b_flags &= ~XBF_DONE;
Shouldn't this condition trigger if either the crc or buffer
verification fails (not if both fail)?
Also, xfs_buf_ioend() sets XBF_DONE when bp->b_error == 0 after the read
verifier is invoked. I don't see bp->b_error being set here, so it looks
like clearing this flag wouldn't have any effect.
Brian
> +}
> +
> +/*
> * we don't calculate the CRC here as that is done when the dquot is flushed to
> * the buffer after the update is done. This ensures that the dquot in the
> * buffer always has an up-to-date CRC value.
> @@ -274,7 +289,7 @@ xfs_dquot_buf_write_verify(
> {
> struct xfs_mount *mp = bp->b_target->bt_mount;
>
> - if (!xfs_dquot_buf_verify(mp, bp)) {
> + if (!xfs_dquot_buf_verify(mp, bp, XFS_QMOPT_DOWARN)) {
> xfs_buf_ioerror(bp, -EFSCORRUPTED);
> xfs_verifier_error(bp);
> return;
> @@ -287,3 +302,8 @@ const struct xfs_buf_ops xfs_dquot_buf_ops = {
> .verify_write = xfs_dquot_buf_write_verify,
> };
>
> +const struct xfs_buf_ops xfs_dquot_buf_ra_ops = {
> + .name = "xfs_dquot_ra",
> + .verify_read = xfs_dquot_buf_readahead_verify,
> + .verify_write = xfs_dquot_buf_write_verify,
> +};
> diff --git a/fs/xfs/libxfs/xfs_quota_defs.h b/fs/xfs/libxfs/xfs_quota_defs.h
> index 1b0a083..f51078f 100644
> --- a/fs/xfs/libxfs/xfs_quota_defs.h
> +++ b/fs/xfs/libxfs/xfs_quota_defs.h
> @@ -153,7 +153,7 @@ typedef __uint16_t xfs_qwarncnt_t;
> #define XFS_QMOPT_RESBLK_MASK (XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_RES_RTBLKS)
>
> extern int xfs_dqcheck(struct xfs_mount *mp, xfs_disk_dquot_t *ddq,
> - xfs_dqid_t id, uint type, uint flags, char *str);
> + xfs_dqid_t id, uint type, uint flags, const char *str);
> extern int xfs_calc_dquots_per_chunk(unsigned int nbblks);
>
> #endif /* __XFS_QUOTA_H__ */
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 5be5297..15c3ceb 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -49,6 +49,7 @@ extern const struct xfs_buf_ops xfs_inobt_buf_ops;
> extern const struct xfs_buf_ops xfs_inode_buf_ops;
> extern const struct xfs_buf_ops xfs_inode_buf_ra_ops;
> extern const struct xfs_buf_ops xfs_dquot_buf_ops;
> +extern const struct xfs_buf_ops xfs_dquot_buf_ra_ops;
> extern const struct xfs_buf_ops xfs_sb_buf_ops;
> extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
> extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 26e67b4..da37beb 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3521,6 +3521,7 @@ xlog_recover_dquot_ra_pass2(
> struct xfs_disk_dquot *recddq;
> struct xfs_dq_logformat *dq_f;
> uint type;
> + int len;
>
>
> if (mp->m_qflags == 0)
> @@ -3541,8 +3542,12 @@ xlog_recover_dquot_ra_pass2(
> ASSERT(dq_f);
> ASSERT(dq_f->qlf_len == 1);
>
> - xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno,
> - XFS_FSB_TO_BB(mp, dq_f->qlf_len), NULL);
> + len = XFS_FSB_TO_BB(mp, dq_f->qlf_len);
> + if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0))
> + return;
> +
> + xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
> + &xfs_dquot_buf_ra_ops);
> }
>
> STATIC void
> --
> 2.5.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
