| Message ID | 20200522035029.3022405-4-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: rework inode flushing to make inode reclaim fully asynchronous | expand |
On Fri, May 22, 2020 at 6:51 AM Dave Chinner <david@fromorbit.com> wrote: > > From: Dave Chinner <dchinner@redhat.com> > > Inode buffers always have write IO callbacks, so by marking them > directly we can avoid needing to attach ->b_iodone functions to > them. This avoids an indirect call, and makes future modifications > much simpler. > > This is largely a rearrangement of the code at this point - no IO > completion functionality changes at this point, just how the > code is run is modified. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_buf.c | 18 +++++++++++++----- > fs/xfs/xfs_buf.h | 39 ++++++++++++++++++++++++++------------- > fs/xfs/xfs_buf_item.c | 42 +++++++++++++++++++++++++++++++----------- > fs/xfs/xfs_buf_item.h | 1 + > fs/xfs/xfs_inode.c | 2 +- > fs/xfs/xfs_trans_buf.c | 3 +++ > 6 files changed, 75 insertions(+), 30 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 9c2fbb6bbf89d..6105b97028d6a 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -14,6 +14,8 @@ > #include "xfs_mount.h" > #include "xfs_trace.h" > #include "xfs_log.h" > +#include "xfs_trans.h" > +#include "xfs_buf_item.h" > #include "xfs_errortag.h" > #include "xfs_error.h" > > @@ -1202,12 +1204,18 @@ xfs_buf_ioend( > bp->b_flags |= XBF_DONE; > } > > - if (bp->b_iodone) > + /* inodes always have a callback on write */ > + if (!read && (bp->b_flags & _XBF_INODES)) { > + xfs_buf_inode_iodone(bp); > + return; > + } > + > + if (bp->b_iodone) { > (*(bp->b_iodone))(bp); > - else if (bp->b_flags & XBF_ASYNC) > - xfs_buf_relse(bp); > - else > - complete(&bp->b_iowait); > + return; > + } > + > + xfs_buf_ioend_finish(bp); > } > > static void > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h > index 050c53b739e24..b3e5d653d09f1 100644 > --- a/fs/xfs/xfs_buf.h > +++ b/fs/xfs/xfs_buf.h > @@ -30,15 +30,19 @@ > #define XBF_STALE (1 << 6) /* buffer has been staled, do not find it */ > #define XBF_WRITE_FAIL (1 << 7) /* async writes have failed on this buffer */ > > -/* flags used only as arguments to access routines */ > -#define XBF_TRYLOCK (1 << 16)/* lock requested, but do not wait */ > -#define XBF_UNMAPPED (1 << 17)/* do not map the buffer */ > +/* buffer type flags for write callbacks */ > +#define _XBF_INODES (1 << 16)/* inode buffer */ As I wrote on review of another type flag, best add a definition of XBF_BUFFER_TYPE_MASK and document that buffer type flags are mutually exclusive, maybe even ASSERT it in some places. For not changing logic by rearranging code: Reviewed-by: Amir Goldstein <amir73il@gmail.com> Thanks, Amir.
On Fri, May 22, 2020 at 01:50:08PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Inode buffers always have write IO callbacks, so by marking them > directly we can avoid needing to attach ->b_iodone functions to > them. This avoids an indirect call, and makes future modifications > much simpler. > > This is largely a rearrangement of the code at this point - no IO > completion functionality changes at this point, just how the > code is run is modified. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_buf.c | 18 +++++++++++++----- > fs/xfs/xfs_buf.h | 39 ++++++++++++++++++++++++++------------- > fs/xfs/xfs_buf_item.c | 42 +++++++++++++++++++++++++++++++----------- > fs/xfs/xfs_buf_item.h | 1 + > fs/xfs/xfs_inode.c | 2 +- > fs/xfs/xfs_trans_buf.c | 3 +++ > 6 files changed, 75 insertions(+), 30 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 9c2fbb6bbf89d..6105b97028d6a 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -14,6 +14,8 @@ > #include "xfs_mount.h" > #include "xfs_trace.h" > #include "xfs_log.h" > +#include "xfs_trans.h" > +#include "xfs_buf_item.h" > #include "xfs_errortag.h" > #include "xfs_error.h" > > @@ -1202,12 +1204,18 @@ xfs_buf_ioend( > bp->b_flags |= XBF_DONE; > } > > - if (bp->b_iodone) > + /* inodes always have a callback on write */ > + if (!read && (bp->b_flags & _XBF_INODES)) { I think this changes in the next patch. > + xfs_buf_inode_iodone(bp); > + return; > + } > + > + if (bp->b_iodone) { > (*(bp->b_iodone))(bp); > - else if (bp->b_flags & XBF_ASYNC) > - xfs_buf_relse(bp); > - else > - complete(&bp->b_iowait); > + return; > + } > + > + xfs_buf_ioend_finish(bp); > } > > static void > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h > index 050c53b739e24..b3e5d653d09f1 100644 > --- a/fs/xfs/xfs_buf.h > +++ b/fs/xfs/xfs_buf.h > @@ -30,15 +30,19 @@ > #define XBF_STALE (1 << 6) /* buffer has been staled, do not find it */ > #define XBF_WRITE_FAIL (1 << 7) /* async writes have failed on this buffer */ > > -/* flags used only as arguments to access routines */ > -#define XBF_TRYLOCK (1 << 16)/* lock requested, but do not wait */ > -#define XBF_UNMAPPED (1 << 17)/* do not map the buffer */ > +/* buffer type flags for write callbacks */ > +#define _XBF_INODES (1 << 16)/* inode buffer */ > > /* flags used only internally */ > #define _XBF_PAGES (1 << 20)/* backed by refcounted pages */ > #define _XBF_KMEM (1 << 21)/* backed by heap memory */ > #define _XBF_DELWRI_Q (1 << 22)/* buffer on a delwri queue */ > > +/* flags used only as arguments to access routines */ > +#define XBF_TRYLOCK (1 << 30)/* lock requested, but do not wait */ > +#define XBF_UNMAPPED (1 << 31)/* do not map the buffer */ > + > + Double newline? > typedef unsigned int xfs_buf_flags_t; > > #define XFS_BUF_FLAGS \ > @@ -50,12 +54,13 @@ typedef unsigned int xfs_buf_flags_t; > { XBF_DONE, "DONE" }, \ > { XBF_STALE, "STALE" }, \ > { XBF_WRITE_FAIL, "WRITE_FAIL" }, \ > - { XBF_TRYLOCK, "TRYLOCK" }, /* should never be set */\ > - { XBF_UNMAPPED, "UNMAPPED" }, /* ditto */\ > + { _XBF_INODES, "INODES" }, \ This a toughie. On the one hand if you're going to go introducing what amounts to two-bit buffer io completion type in the middle of b_flags then (like Amir says) this ideally would have a mask and switch statements and whatnot. I also wonder if we could tell the buffer type given all the xfs_trans_buf_set_type calls, but I think the answer is that not every buffer is guaranteed to have a buffer log item attached and a type code set correctly? OTOH there's only three states, so who cares, maybe this is fine... --D > { _XBF_PAGES, "PAGES" }, \ > { _XBF_KMEM, "KMEM" }, \ > - { _XBF_DELWRI_Q, "DELWRI_Q" } > - > + { _XBF_DELWRI_Q, "DELWRI_Q" }, \ > + /* The following interface flags should never be set */ \ > + { XBF_TRYLOCK, "TRYLOCK" }, \ > + { XBF_UNMAPPED, "UNMAPPED" } > > /* > * Internal state flags. > @@ -257,9 +262,23 @@ extern void xfs_buf_unlock(xfs_buf_t *); > #define xfs_buf_islocked(bp) \ > ((bp)->b_sema.count <= 0) > > +static inline void xfs_buf_relse(xfs_buf_t *bp) > +{ > + xfs_buf_unlock(bp); > + xfs_buf_rele(bp); > +} > + > /* Buffer Read and Write Routines */ > extern int xfs_bwrite(struct xfs_buf *bp); > extern void xfs_buf_ioend(struct xfs_buf *bp); > +static inline void xfs_buf_ioend_finish(struct xfs_buf *bp) > +{ > + if (bp->b_flags & XBF_ASYNC) > + xfs_buf_relse(bp); > + else > + complete(&bp->b_iowait); > +} > + > extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error, > xfs_failaddr_t failaddr); > #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address) > @@ -324,12 +343,6 @@ static inline int xfs_buf_ispinned(struct xfs_buf *bp) > return atomic_read(&bp->b_pin_count); > } > > -static inline void xfs_buf_relse(xfs_buf_t *bp) > -{ > - xfs_buf_unlock(bp); > - xfs_buf_rele(bp); > -} > - > static inline int > xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset) > { > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c > index 9e75e8d6042ec..8659cf4282a64 100644 > --- a/fs/xfs/xfs_buf_item.c > +++ b/fs/xfs/xfs_buf_item.c > @@ -1158,20 +1158,15 @@ xfs_buf_iodone_callback_error( > return false; > } > > -/* > - * This is the iodone() function for buffers which have had callbacks attached > - * to them by xfs_buf_attach_iodone(). We need to iterate the items on the > - * callback list, mark the buffer as having no more callbacks and then push the > - * buffer through IO completion processing. > - */ > -void > -xfs_buf_iodone_callbacks( > +static void > +xfs_buf_run_callbacks( > struct xfs_buf *bp) > { > + > /* > - * If there is an error, process it. Some errors require us > - * to run callbacks after failure processing is done so we > - * detect that and take appropriate action. > + * If there is an error, process it. Some errors require us to run > + * callbacks after failure processing is done so we detect that and take > + * appropriate action. > */ > if (bp->b_error && xfs_buf_iodone_callback_error(bp)) > return; > @@ -1188,9 +1183,34 @@ xfs_buf_iodone_callbacks( > bp->b_log_item = NULL; > list_del_init(&bp->b_li_list); > bp->b_iodone = NULL; > +} > + > +/* > + * This is the iodone() function for buffers which have had callbacks attached > + * to them by xfs_buf_attach_iodone(). We need to iterate the items on the > + * callback list, mark the buffer as having no more callbacks and then push the > + * buffer through IO completion processing. > + */ > +void > +xfs_buf_iodone_callbacks( > + struct xfs_buf *bp) > +{ > + xfs_buf_run_callbacks(bp); > xfs_buf_ioend(bp); > } > > +/* > + * Inode buffer iodone callback function. > + */ > +void > +xfs_buf_inode_iodone( > + struct xfs_buf *bp) > +{ > + xfs_buf_run_callbacks(bp); > + xfs_buf_ioend_finish(bp); > +} > + > + > /* > * This is the iodone() function for buffers which have been > * logged. It is called when they are eventually flushed out. > diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h > index c9c57e2da9327..a342933ad9b8d 100644 > --- a/fs/xfs/xfs_buf_item.h > +++ b/fs/xfs/xfs_buf_item.h > @@ -59,6 +59,7 @@ void xfs_buf_attach_iodone(struct xfs_buf *, > struct xfs_log_item *); > void xfs_buf_iodone_callbacks(struct xfs_buf *); > void xfs_buf_iodone(struct xfs_buf *, struct xfs_log_item *); > +void xfs_buf_inode_iodone(struct xfs_buf *); > bool xfs_buf_log_check_iovec(struct xfs_log_iovec *iovec); > > extern kmem_zone_t *xfs_buf_item_zone; > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 57781c0dbbec5..607c9d9bb2b40 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -3841,13 +3841,13 @@ xfs_iflush_int( > * completion on the buffer to remove the inode from the AIL and release > * the flush lock. > */ > + bp->b_flags |= _XBF_INODES; > xfs_buf_attach_iodone(bp, xfs_iflush_done, &iip->ili_item); > > /* generate the checksum. */ > xfs_dinode_calc_crc(mp, dip); > > ASSERT(!list_empty(&bp->b_li_list)); > - ASSERT(bp->b_iodone != NULL); > return error; > } > > diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c > index 08174ffa21189..552d0869aa0fe 100644 > --- a/fs/xfs/xfs_trans_buf.c > +++ b/fs/xfs/xfs_trans_buf.c > @@ -626,6 +626,7 @@ xfs_trans_inode_buf( > ASSERT(atomic_read(&bip->bli_refcount) > 0); > > bip->bli_flags |= XFS_BLI_INODE_BUF; > + bp->b_flags |= _XBF_INODES; > xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); > } > > @@ -651,6 +652,7 @@ xfs_trans_stale_inode_buf( > > bip->bli_flags |= XFS_BLI_STALE_INODE; > bip->bli_item.li_cb = xfs_buf_iodone; > + bp->b_flags |= _XBF_INODES; > xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); > } > > @@ -675,6 +677,7 @@ xfs_trans_inode_alloc_buf( > ASSERT(atomic_read(&bip->bli_refcount) > 0); > > bip->bli_flags |= XFS_BLI_INODE_ALLOC_BUF; > + bp->b_flags |= _XBF_INODES; > xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); > } > > -- > 2.26.2.761.g0e0b3e54be >
On Fri, May 22, 2020 at 01:50:08PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Inode buffers always have write IO callbacks, so by marking them > directly we can avoid needing to attach ->b_iodone functions to > them. This avoids an indirect call, and makes future modifications > much simpler. > > This is largely a rearrangement of the code at this point - no IO > completion functionality changes at this point, just how the > code is run is modified. So I've looked over the whole series where this leads, and I really like killing off li_cb and sorting out the mess around b_iodone. But I think the series ends up moving too much out of xfs_buf.c. I'd rather keep a minimal b_write_done callback rather than the checking of the inode and dquote flags, and keep most of the logic in xfs_buf.c. This is the patch I cooked up on top of your series to show what I mean - it removes the need for both flags and kills about 100 lines of code: diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c index e5f58a4b13eee..4220fee1c9ddb 100644 --- a/fs/xfs/libxfs/xfs_trans_inode.c +++ b/fs/xfs/libxfs/xfs_trans_inode.c @@ -169,7 +169,7 @@ xfs_trans_log_inode( xfs_buf_hold(bp); spin_lock(&iip->ili_lock); iip->ili_item.li_buf = bp; - bp->b_flags |= _XBF_INODES; + bp->b_write_done = xfs_iflush_done; list_add_tail(&iip->ili_item.li_bio_list, &bp->b_li_list); xfs_trans_brelse(tp, bp); } diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 5fc83637f7aff..b1c3d8076d52d 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1172,11 +1172,7 @@ xfs_buf_wait_unpin( set_current_state(TASK_RUNNING); } -/* - * Buffer Utility Routines - */ - -void +static void xfs_buf_ioend( struct xfs_buf *bp) { @@ -1198,38 +1194,41 @@ xfs_buf_ioend( bp->b_ops->verify_read(bp); if (!bp->b_error) bp->b_flags |= XBF_DONE; - xfs_buf_ioend_finish(bp); - return; - } - - if (!bp->b_error) { - bp->b_flags &= ~XBF_WRITE_FAIL; - bp->b_flags |= XBF_DONE; - } - - /* - * If this is a log recovery buffer, we aren't doing transactional IO - * yet so we need to let it handle IO completions. - */ - if (bp->b_flags & _XBF_LOGRCVY) { + } else if (unlikely(bp->b_flags & _XBF_LOGRCVY)) { xlog_recover_iodone(bp); - return; - } - - /* inodes always have a callback on write */ - if (bp->b_flags & _XBF_INODES) { - xfs_buf_inode_iodone(bp); - return; - } + } else { + /* + * If there is an error, process it. Some errors require us to + * run callbacks after failure processing is done so we detect + * that and take appropriate action. + */ + if (bp->b_error) { + if (xfs_buf_iodone_callback_error(bp)) + return; + } else { + bp->b_flags &= ~XBF_WRITE_FAIL; + bp->b_flags |= XBF_DONE; + } - /* dquots always have a callback on write */ - if (bp->b_flags & _XBF_DQUOTS) { - xfs_buf_dquot_iodone(bp); - return; + /* + * Successful IO or permanent error. Either way, we can clear + * the retry state here in preparation for the next error that + * may occur. + */ + bp->b_last_error = 0; + bp->b_retries = 0; + bp->b_first_retry_time = 0; + + if (bp->b_write_done) + bp->b_write_done(bp); + if (bp->b_log_item) + xfs_buf_item_done(bp); } - /* dirty buffers always need to be removed from the AIL */ - xfs_buf_dirty_iodone(bp); + if (bp->b_flags & XBF_ASYNC) + xfs_buf_relse(bp); + else + complete(&bp->b_iowait); } static void diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index a127d56d5eff0..a060d8bfeb515 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -32,9 +32,7 @@ struct xfs_buf; #define XBF_WRITE_FAIL (1 << 7) /* async writes have failed on this buffer */ /* buffer type flags for write callbacks */ -#define _XBF_INODES (1 << 16)/* inode buffer */ -#define _XBF_DQUOTS (1 << 17)/* dquot buffer */ -#define _XBF_LOGRCVY (1 << 18)/* log recovery buffer */ +#define _XBF_LOGRCVY (1 << 18)/* log recovery buffer */ /* flags used only internally */ #define _XBF_PAGES (1 << 20)/* backed by refcounted pages */ @@ -57,8 +55,6 @@ typedef unsigned int xfs_buf_flags_t; { XBF_DONE, "DONE" }, \ { XBF_STALE, "STALE" }, \ { XBF_WRITE_FAIL, "WRITE_FAIL" }, \ - { _XBF_INODES, "INODES" }, \ - { _XBF_DQUOTS, "DQUOTS" }, \ { _XBF_LOGRCVY, "LOG_RECOVERY" }, \ { _XBF_PAGES, "PAGES" }, \ { _XBF_KMEM, "KMEM" }, \ @@ -156,6 +152,7 @@ typedef struct xfs_buf { xfs_buftarg_t *b_target; /* buffer target (device) */ void *b_addr; /* virtual address of buffer */ struct work_struct b_ioend_work; + void (*b_write_done)(struct xfs_buf *bp); struct completion b_iowait; /* queue for I/O waiters */ struct xfs_buf_log_item *b_log_item; struct list_head b_li_list; /* Log items list head */ @@ -262,23 +259,8 @@ extern void xfs_buf_unlock(xfs_buf_t *); #define xfs_buf_islocked(bp) \ ((bp)->b_sema.count <= 0) -static inline void xfs_buf_relse(xfs_buf_t *bp) -{ - xfs_buf_unlock(bp); - xfs_buf_rele(bp); -} - /* Buffer Read and Write Routines */ extern int xfs_bwrite(struct xfs_buf *bp); -extern void xfs_buf_ioend(struct xfs_buf *bp); -static inline void xfs_buf_ioend_finish(struct xfs_buf *bp) -{ - if (bp->b_flags & XBF_ASYNC) - xfs_buf_relse(bp); - else - complete(&bp->b_iowait); -} - extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error, xfs_failaddr_t failaddr); #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address) @@ -343,6 +325,12 @@ static inline int xfs_buf_ispinned(struct xfs_buf *bp) return atomic_read(&bp->b_pin_count); } +static inline void xfs_buf_relse(xfs_buf_t *bp) +{ + xfs_buf_unlock(bp); + xfs_buf_rele(bp); +} + static inline int xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset) { diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c index d855a2b7486c5..21b0d2f3f1af9 100644 --- a/fs/xfs/xfs_buf_item.c +++ b/fs/xfs/xfs_buf_item.c @@ -15,9 +15,6 @@ #include "xfs_buf_item.h" #include "xfs_inode.h" #include "xfs_inode_item.h" -#include "xfs_quota.h" -#include "xfs_dquot_item.h" -#include "xfs_dquot.h" #include "xfs_trans_priv.h" #include "xfs_trace.h" #include "xfs_log.h" @@ -30,8 +27,6 @@ static inline struct xfs_buf_log_item *BUF_ITEM(struct xfs_log_item *lip) return container_of(lip, struct xfs_buf_log_item, bli_item); } -static void xfs_buf_item_done(struct xfs_buf *bp); - /* Is this log iovec plausibly large enough to contain the buffer log format? */ bool xfs_buf_log_check_iovec( @@ -986,7 +981,7 @@ xfs_buf_do_callbacks_fail( spin_unlock(&ailp->ail_lock); } -static bool +bool xfs_buf_iodone_callback_error( struct xfs_buf *bp) { @@ -1075,38 +1070,12 @@ xfs_buf_iodone_callback_error( return false; } -static inline bool -xfs_buf_had_callback_errors( - struct xfs_buf *bp) -{ - - /* - * If there is an error, process it. Some errors require us to run - * callbacks after failure processing is done so we detect that and take - * appropriate action. - */ - if (bp->b_error && xfs_buf_iodone_callback_error(bp)) - return true; - - /* - * Successful IO or permanent error. Either way, we can clear the - * retry state here in preparation for the next error that may occur. - */ - bp->b_last_error = 0; - bp->b_retries = 0; - bp->b_first_retry_time = 0; - return false; -} - -static void +void xfs_buf_item_done( struct xfs_buf *bp) { struct xfs_buf_log_item *bip = bp->b_log_item; - if (!bip) - return; - /* * If we are forcibly shutting down, this may well be off the AIL * already. That's because we simulate the log-committed callbacks to @@ -1121,52 +1090,3 @@ xfs_buf_item_done( xfs_buf_item_free(bip); xfs_buf_rele(bp); } - -/* - * Inode buffer iodone callback function. - */ -void -xfs_buf_inode_iodone( - struct xfs_buf *bp) -{ - if (xfs_buf_had_callback_errors(bp)) - return; - - xfs_buf_item_done(bp); - xfs_iflush_done(bp); - xfs_buf_ioend_finish(bp); -} - -/* - * Dquot buffer iodone callback function. - */ -void -xfs_buf_dquot_iodone( - struct xfs_buf *bp) -{ - if (xfs_buf_had_callback_errors(bp)) - return; - - /* a newly allocated dquot buffer might have a log item attached */ - xfs_buf_item_done(bp); - xfs_dquot_done(bp); - xfs_buf_ioend_finish(bp); -} - -/* - * Dirty buffer iodone callback function. - * - * Note that for things like remote attribute buffers, there may not be a buffer - * log item here, so processing the buffer log item must remain be optional. - */ -void -xfs_buf_dirty_iodone( - struct xfs_buf *bp) -{ - if (xfs_buf_had_callback_errors(bp)) - return; - - xfs_buf_item_done(bp); - xfs_buf_ioend_finish(bp); -} - diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h index ecfad1915a86b..4ffeabd3ad967 100644 --- a/fs/xfs/xfs_buf_item.h +++ b/fs/xfs/xfs_buf_item.h @@ -54,11 +54,11 @@ void xfs_buf_item_relse(struct xfs_buf *); bool xfs_buf_item_put(struct xfs_buf_log_item *); void xfs_buf_item_log(struct xfs_buf_log_item *, uint, uint); bool xfs_buf_item_dirty_format(struct xfs_buf_log_item *); -void xfs_buf_inode_iodone(struct xfs_buf *); -void xfs_buf_dquot_iodone(struct xfs_buf *); -void xfs_buf_dirty_iodone(struct xfs_buf *); bool xfs_buf_log_check_iovec(struct xfs_log_iovec *iovec); +bool xfs_buf_iodone_callback_error(struct xfs_buf *bp); +void xfs_buf_item_done(struct xfs_buf *bp); + extern kmem_zone_t *xfs_buf_item_zone; #endif /* __XFS_BUF_ITEM_H__ */ diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index 34fc1bcb1eefd..55f34c60340c0 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -1085,7 +1085,7 @@ xfs_qm_dqflush_done( xfs_dqfunlock(dqp); } -void +static void xfs_dquot_done( struct xfs_buf *bp) { @@ -1194,7 +1194,7 @@ xfs_qm_dqflush( * Attach the dquot to the buffer so that we can remove this dquot from * the AIL and release the flush lock once the dquot is synced to disk. */ - bp->b_flags |= _XBF_DQUOTS; + bp->b_write_done = xfs_dquot_done; list_add_tail(&dqp->q_logitem.qli_item.li_bio_list, &bp->b_li_list); /* diff --git a/fs/xfs/xfs_dquot.h b/fs/xfs/xfs_dquot.h index f1924288986d3..fe3e46df604b4 100644 --- a/fs/xfs/xfs_dquot.h +++ b/fs/xfs/xfs_dquot.h @@ -174,7 +174,6 @@ void xfs_qm_dqput(struct xfs_dquot *dqp); void xfs_dqlock2(struct xfs_dquot *, struct xfs_dquot *); void xfs_dquot_set_prealloc_limits(struct xfs_dquot *); -void xfs_dquot_done(struct xfs_buf *); static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp) { diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 0aa823aeafca9..5a1dbcde6e9f0 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -269,15 +269,15 @@ void xlog_recover_iodone( struct xfs_buf *bp) { - if (bp->b_error) { - /* - * We're not going to bother about retrying - * this during recovery. One strike! - */ - if (!XFS_FORCED_SHUTDOWN(bp->b_mount)) { - xfs_buf_ioerror_alert(bp, __this_address); - xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); - } + /* + * We're not going to bother about retrying this during recovery. + * One strike! + */ + if (!bp->b_error) { + bp->b_flags |= XBF_DONE; + } else if (!XFS_FORCED_SHUTDOWN(bp->b_mount)) { + xfs_buf_ioerror_alert(bp, __this_address); + xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); } /* @@ -288,7 +288,6 @@ xlog_recover_iodone( xfs_buf_item_relse(bp); ASSERT(bp->b_log_item == NULL); bp->b_flags &= ~_XBF_LOGRCVY; - xfs_buf_ioend_finish(bp); } /* diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c index 11cd666cd99a6..3a2fe19832d89 100644 --- a/fs/xfs/xfs_trans_buf.c +++ b/fs/xfs/xfs_trans_buf.c @@ -618,7 +618,6 @@ xfs_trans_inode_buf( ASSERT(atomic_read(&bip->bli_refcount) > 0); bip->bli_flags |= XFS_BLI_INODE_BUF; - bp->b_flags |= _XBF_INODES; xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); } @@ -643,7 +642,6 @@ xfs_trans_stale_inode_buf( ASSERT(atomic_read(&bip->bli_refcount) > 0); bip->bli_flags |= XFS_BLI_STALE_INODE; - bp->b_flags |= _XBF_INODES; xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); } @@ -668,7 +666,6 @@ xfs_trans_inode_alloc_buf( ASSERT(atomic_read(&bip->bli_refcount) > 0); bip->bli_flags |= XFS_BLI_INODE_ALLOC_BUF; - bp->b_flags |= _XBF_INODES; xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); } @@ -779,6 +776,5 @@ xfs_trans_dquot_buf( break; } - bp->b_flags |= _XBF_DQUOTS; xfs_trans_buf_set_type(tp, bp, type); }
On Fri, May 22, 2020 at 02:35:17PM -0700, Darrick J. Wong wrote: > On Fri, May 22, 2020 at 01:50:08PM +1000, Dave Chinner wrote: > > #define XFS_BUF_FLAGS \ > > @@ -50,12 +54,13 @@ typedef unsigned int xfs_buf_flags_t; > > { XBF_DONE, "DONE" }, \ > > { XBF_STALE, "STALE" }, \ > > { XBF_WRITE_FAIL, "WRITE_FAIL" }, \ > > - { XBF_TRYLOCK, "TRYLOCK" }, /* should never be set */\ > > - { XBF_UNMAPPED, "UNMAPPED" }, /* ditto */\ > > + { _XBF_INODES, "INODES" }, \ > > This a toughie. On the one hand if you're going to go introducing what > amounts to two-bit buffer io completion type in the middle of b_flags > then (like Amir says) this ideally would have a mask and switch > statements and whatnot. I didn't really want a mask, because these are not unconditional "buffer type" flags. They are only set on buffers with inodes attached for IO completion and used that way in this patchset. Everywhere else that uses them only checks for a single type... > I also wonder if we could tell the buffer type given all the > xfs_trans_buf_set_type calls, but I think the answer is that not every > buffer is guaranteed to have a buffer log item attached and a type code > set correctly? Correct - that's what I looked at first, but inode cluster buffers that have not just been created or have an unlinked inode logged against them don't have buffer log items. Similarly dquot buffers may not have a log item, either. Cheers, Dave.
On Sat, May 23, 2020 at 01:48:27AM -0700, Christoph Hellwig wrote: > On Fri, May 22, 2020 at 01:50:08PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > Inode buffers always have write IO callbacks, so by marking them > > directly we can avoid needing to attach ->b_iodone functions to > > them. This avoids an indirect call, and makes future modifications > > much simpler. > > > > This is largely a rearrangement of the code at this point - no IO > > completion functionality changes at this point, just how the > > code is run is modified. > > So I've looked over the whole series where this leads, and I really like > killing off li_cb and sorting out the mess around b_iodone. But I think > the series ends up moving too much out of xfs_buf.c. I'd rather keep > a minimal b_write_done callback rather than the checking of the inode > and dquote flags, and keep most of the logic in xfs_buf.c. This is the > patch I cooked up on top of your series to show what I mean - it removes > the need for both flags and kills about 100 lines of code: Let's deal with further cleanups after this patchset and the follow-on patchsets that I have are sorted out. All cleanups will do right now is invalidate all the stuff I'm still working on that sits on top of this patchset.. > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c > index e5f58a4b13eee..4220fee1c9ddb 100644 > --- a/fs/xfs/libxfs/xfs_trans_inode.c > +++ b/fs/xfs/libxfs/xfs_trans_inode.c > @@ -169,7 +169,7 @@ xfs_trans_log_inode( > xfs_buf_hold(bp); > spin_lock(&iip->ili_lock); > iip->ili_item.li_buf = bp; > - bp->b_flags |= _XBF_INODES; > + bp->b_write_done = xfs_iflush_done; I originally tried this, but then I ended up with places in follow-on patchsets where I had to do this sort of check: if (bp->b_write_done == xfs_iflush_done) To detect a buffer that already had inodes attached to it. Which clearly told me that there should be a state flag to indicate that the buffer has attached inodes.... Cheers, Dave.
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 9c2fbb6bbf89d..6105b97028d6a 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -14,6 +14,8 @@ #include "xfs_mount.h" #include "xfs_trace.h" #include "xfs_log.h" +#include "xfs_trans.h" +#include "xfs_buf_item.h" #include "xfs_errortag.h" #include "xfs_error.h" @@ -1202,12 +1204,18 @@ xfs_buf_ioend( bp->b_flags |= XBF_DONE; } - if (bp->b_iodone) + /* inodes always have a callback on write */ + if (!read && (bp->b_flags & _XBF_INODES)) { + xfs_buf_inode_iodone(bp); + return; + } + + if (bp->b_iodone) { (*(bp->b_iodone))(bp); - else if (bp->b_flags & XBF_ASYNC) - xfs_buf_relse(bp); - else - complete(&bp->b_iowait); + return; + } + + xfs_buf_ioend_finish(bp); } static void diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index 050c53b739e24..b3e5d653d09f1 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -30,15 +30,19 @@ #define XBF_STALE (1 << 6) /* buffer has been staled, do not find it */ #define XBF_WRITE_FAIL (1 << 7) /* async writes have failed on this buffer */ -/* flags used only as arguments to access routines */ -#define XBF_TRYLOCK (1 << 16)/* lock requested, but do not wait */ -#define XBF_UNMAPPED (1 << 17)/* do not map the buffer */ +/* buffer type flags for write callbacks */ +#define _XBF_INODES (1 << 16)/* inode buffer */ /* flags used only internally */ #define _XBF_PAGES (1 << 20)/* backed by refcounted pages */ #define _XBF_KMEM (1 << 21)/* backed by heap memory */ #define _XBF_DELWRI_Q (1 << 22)/* buffer on a delwri queue */ +/* flags used only as arguments to access routines */ +#define XBF_TRYLOCK (1 << 30)/* lock requested, but do not wait */ +#define XBF_UNMAPPED (1 << 31)/* do not map the buffer */ + + typedef unsigned int xfs_buf_flags_t; #define XFS_BUF_FLAGS \ @@ -50,12 +54,13 @@ typedef unsigned int xfs_buf_flags_t; { XBF_DONE, "DONE" }, \ { XBF_STALE, "STALE" }, \ { XBF_WRITE_FAIL, "WRITE_FAIL" }, \ - { XBF_TRYLOCK, "TRYLOCK" }, /* should never be set */\ - { XBF_UNMAPPED, "UNMAPPED" }, /* ditto */\ + { _XBF_INODES, "INODES" }, \ { _XBF_PAGES, "PAGES" }, \ { _XBF_KMEM, "KMEM" }, \ - { _XBF_DELWRI_Q, "DELWRI_Q" } - + { _XBF_DELWRI_Q, "DELWRI_Q" }, \ + /* The following interface flags should never be set */ \ + { XBF_TRYLOCK, "TRYLOCK" }, \ + { XBF_UNMAPPED, "UNMAPPED" } /* * Internal state flags. @@ -257,9 +262,23 @@ extern void xfs_buf_unlock(xfs_buf_t *); #define xfs_buf_islocked(bp) \ ((bp)->b_sema.count <= 0) +static inline void xfs_buf_relse(xfs_buf_t *bp) +{ + xfs_buf_unlock(bp); + xfs_buf_rele(bp); +} + /* Buffer Read and Write Routines */ extern int xfs_bwrite(struct xfs_buf *bp); extern void xfs_buf_ioend(struct xfs_buf *bp); +static inline void xfs_buf_ioend_finish(struct xfs_buf *bp) +{ + if (bp->b_flags & XBF_ASYNC) + xfs_buf_relse(bp); + else + complete(&bp->b_iowait); +} + extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error, xfs_failaddr_t failaddr); #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address) @@ -324,12 +343,6 @@ static inline int xfs_buf_ispinned(struct xfs_buf *bp) return atomic_read(&bp->b_pin_count); } -static inline void xfs_buf_relse(xfs_buf_t *bp) -{ - xfs_buf_unlock(bp); - xfs_buf_rele(bp); -} - static inline int xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset) { diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c index 9e75e8d6042ec..8659cf4282a64 100644 --- a/fs/xfs/xfs_buf_item.c +++ b/fs/xfs/xfs_buf_item.c @@ -1158,20 +1158,15 @@ xfs_buf_iodone_callback_error( return false; } -/* - * This is the iodone() function for buffers which have had callbacks attached - * to them by xfs_buf_attach_iodone(). We need to iterate the items on the - * callback list, mark the buffer as having no more callbacks and then push the - * buffer through IO completion processing. - */ -void -xfs_buf_iodone_callbacks( +static void +xfs_buf_run_callbacks( struct xfs_buf *bp) { + /* - * If there is an error, process it. Some errors require us - * to run callbacks after failure processing is done so we - * detect that and take appropriate action. + * If there is an error, process it. Some errors require us to run + * callbacks after failure processing is done so we detect that and take + * appropriate action. */ if (bp->b_error && xfs_buf_iodone_callback_error(bp)) return; @@ -1188,9 +1183,34 @@ xfs_buf_iodone_callbacks( bp->b_log_item = NULL; list_del_init(&bp->b_li_list); bp->b_iodone = NULL; +} + +/* + * This is the iodone() function for buffers which have had callbacks attached + * to them by xfs_buf_attach_iodone(). We need to iterate the items on the + * callback list, mark the buffer as having no more callbacks and then push the + * buffer through IO completion processing. + */ +void +xfs_buf_iodone_callbacks( + struct xfs_buf *bp) +{ + xfs_buf_run_callbacks(bp); xfs_buf_ioend(bp); } +/* + * Inode buffer iodone callback function. + */ +void +xfs_buf_inode_iodone( + struct xfs_buf *bp) +{ + xfs_buf_run_callbacks(bp); + xfs_buf_ioend_finish(bp); +} + + /* * This is the iodone() function for buffers which have been * logged. It is called when they are eventually flushed out. diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h index c9c57e2da9327..a342933ad9b8d 100644 --- a/fs/xfs/xfs_buf_item.h +++ b/fs/xfs/xfs_buf_item.h @@ -59,6 +59,7 @@ void xfs_buf_attach_iodone(struct xfs_buf *, struct xfs_log_item *); void xfs_buf_iodone_callbacks(struct xfs_buf *); void xfs_buf_iodone(struct xfs_buf *, struct xfs_log_item *); +void xfs_buf_inode_iodone(struct xfs_buf *); bool xfs_buf_log_check_iovec(struct xfs_log_iovec *iovec); extern kmem_zone_t *xfs_buf_item_zone; diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 57781c0dbbec5..607c9d9bb2b40 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -3841,13 +3841,13 @@ xfs_iflush_int( * completion on the buffer to remove the inode from the AIL and release * the flush lock. */ + bp->b_flags |= _XBF_INODES; xfs_buf_attach_iodone(bp, xfs_iflush_done, &iip->ili_item); /* generate the checksum. */ xfs_dinode_calc_crc(mp, dip); ASSERT(!list_empty(&bp->b_li_list)); - ASSERT(bp->b_iodone != NULL); return error; } diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c index 08174ffa21189..552d0869aa0fe 100644 --- a/fs/xfs/xfs_trans_buf.c +++ b/fs/xfs/xfs_trans_buf.c @@ -626,6 +626,7 @@ xfs_trans_inode_buf( ASSERT(atomic_read(&bip->bli_refcount) > 0); bip->bli_flags |= XFS_BLI_INODE_BUF; + bp->b_flags |= _XBF_INODES; xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); } @@ -651,6 +652,7 @@ xfs_trans_stale_inode_buf( bip->bli_flags |= XFS_BLI_STALE_INODE; bip->bli_item.li_cb = xfs_buf_iodone; + bp->b_flags |= _XBF_INODES; xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); } @@ -675,6 +677,7 @@ xfs_trans_inode_alloc_buf( ASSERT(atomic_read(&bip->bli_refcount) > 0); bip->bli_flags |= XFS_BLI_INODE_ALLOC_BUF; + bp->b_flags |= _XBF_INODES; xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF); }