[Top] [All Lists]

Re: [PATCH 22/23] xfs: add pre-write metadata buffer verifier callbacks

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 22/23] xfs: add pre-write metadata buffer verifier callbacks
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sat, 13 Oct 2012 12:02:12 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1350020669-19504-23-git-send-email-david@xxxxxxxxxxxxx>
References: <1350020669-19504-1-git-send-email-david@xxxxxxxxxxxxx> <1350020669-19504-23-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
> +xfs_agfl_write_verify(
> +     struct xfs_buf  *bp)
> +{
> +     xfs_agfl_verify(bp);
> +}
> +
> +void
> +xfs_agfl_read_verify(
> +     struct xfs_buf  *bp)
> +{
> +     xfs_agfl_verify(bp);
> +     bp->b_pre_io = xfs_agfl_write_verify;
>       bp->b_iodone = NULL;
>       xfs_buf_ioend(bp, 0);

I have to say I hate the way the API works for the buffer callbacks
even more now that I see the write side.  I know you're a bit annoyed
about churn from review requests, but I'd really prefer if this
was redone.

Two ways to do this nicer come to mind:

 - Have one commone b_verify callback, which gets a bool for_write
   argument to key of differences.  The xfs_buf_ioend call for reads
   remains in the caller, b_iodone is not touched at all.  This will
   remove the boilerplate code a lot in the current version.
 - Expecting to have some more difference between the read and the
   write side when we actually do the crcs from my work on the previous
   iteration of it it might make sense to have two callbacks, but I'd
   again prefer to not overload b_iodone.  Maybe just pass a:

struct xfs_buf_ops {
        int (*verify_read)(struct xfs_buf *);
        int (*verify_write)(struct xfs_buf *);

to all the buffer read/write API calls, and let that deal with it.

<Prev in Thread] Current Thread [Next in Thread>