| Message ID | 20200522035029.3022405-6-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> > > Log recovery has it's own buffer write completion handler for > buffers that it directly recovers. Convert these to direct calls by > flagging these buffers as being log recovery buffers. The flag will > get cleared by the log recovery IO completion routine, so it will > never leak out of log recovery. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_buf.c | 10 ++++++++++ > fs/xfs/xfs_buf.h | 2 ++ > fs/xfs/xfs_buf_item_recover.c | 5 ++--- > fs/xfs/xfs_dquot_item_recover.c | 2 +- > fs/xfs/xfs_inode_item_recover.c | 2 +- > fs/xfs/xfs_log_recover.c | 5 ++--- > 6 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 77d40eb4a11db..b89685ce8519d 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -14,6 +14,7 @@ > #include "xfs_mount.h" > #include "xfs_trace.h" > #include "xfs_log.h" > +#include "xfs_log_recover.h" > #include "xfs_trans.h" > #include "xfs_buf_item.h" > #include "xfs_errortag.h" > @@ -1207,6 +1208,15 @@ xfs_buf_ioend( > if (read) > goto out_finish; > > + /* > + * 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) { > + xlog_recover_iodone(bp); > + return; > + } > + > /* inodes always have a callback on write */ > if (bp->b_flags & _XBF_INODES) { > xfs_buf_inode_iodone(bp); This turns out to be a "static calls" pattern. I think it would look nicer as a switch statement on (bp->b_flags & _XBF_BUFFER_TYPE_MASK) It would be also nicer to document near flag definition that the type flags are mutually exclusive. > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h > index cbde44ecb3963..c5fe4c48c9080 100644 > --- a/fs/xfs/xfs_buf.h > +++ b/fs/xfs/xfs_buf.h > @@ -33,6 +33,7 @@ > /* 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 */ > > /* flags used only internally */ > #define _XBF_PAGES (1 << 20)/* backed by refcounted pages */ > @@ -57,6 +58,7 @@ typedef unsigned int xfs_buf_flags_t; > { XBF_WRITE_FAIL, "WRITE_FAIL" }, \ > { _XBF_INODES, "INODES" }, \ > { _XBF_DQUOTS, "DQUOTS" }, \ > + { _XBF_LOGRCVY, "LOG_RECOVERY" }, \ > { _XBF_PAGES, "PAGES" }, \ > { _XBF_KMEM, "KMEM" }, \ > { _XBF_DELWRI_Q, "DELWRI_Q" }, \ > diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c > index 04faa7310c4f0..bfd50daa16606 100644 > --- a/fs/xfs/xfs_buf_item_recover.c > +++ b/fs/xfs/xfs_buf_item_recover.c > @@ -419,8 +419,7 @@ xlog_recover_validate_buf_type( > if (bp->b_ops) { > struct xfs_buf_log_item *bip; > > - ASSERT(!bp->b_iodone || bp->b_iodone == xlog_recover_iodone); > - bp->b_iodone = xlog_recover_iodone; > + bp->b_flags |= _XBF_LOGRCVY; > xfs_buf_item_init(bp, mp); > bip = bp->b_log_item; > bip->bli_item.li_lsn = current_lsn; > @@ -963,7 +962,7 @@ xlog_recover_buf_commit_pass2( > error = xfs_bwrite(bp); > } else { > ASSERT(bp->b_mount == mp); > - bp->b_iodone = xlog_recover_iodone; > + bp->b_flags |= _XBF_LOGRCVY; > xfs_buf_delwri_queue(bp, buffer_list); > } > > diff --git a/fs/xfs/xfs_dquot_item_recover.c b/fs/xfs/xfs_dquot_item_recover.c > index 3400be4c88f08..a0a4b089e0cdd 100644 > --- a/fs/xfs/xfs_dquot_item_recover.c > +++ b/fs/xfs/xfs_dquot_item_recover.c > @@ -153,7 +153,7 @@ xlog_recover_dquot_commit_pass2( > > ASSERT(dq_f->qlf_size == 2); > ASSERT(bp->b_mount == mp); > - bp->b_iodone = xlog_recover_iodone; > + bp->b_flags |= _XBF_LOGRCVY; > xfs_buf_delwri_queue(bp, buffer_list); > > out_release: > diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c > index dc3e26ff16c90..b67f1b7c5b65f 100644 > --- a/fs/xfs/xfs_inode_item_recover.c > +++ b/fs/xfs/xfs_inode_item_recover.c > @@ -376,7 +376,7 @@ xlog_recover_inode_commit_pass2( > xfs_dinode_calc_crc(log->l_mp, dip); > > ASSERT(bp->b_mount == mp); > - bp->b_iodone = xlog_recover_iodone; > + bp->b_flags |= _XBF_LOGRCVY; > xfs_buf_delwri_queue(bp, buffer_list); > > out_release: > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index ec015df55b77a..0aa823aeafca9 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -287,9 +287,8 @@ xlog_recover_iodone( > if (bp->b_log_item) > xfs_buf_item_relse(bp); > ASSERT(bp->b_log_item == NULL); > - > - bp->b_iodone = NULL; > - xfs_buf_ioend(bp); > + bp->b_flags &= ~_XBF_LOGRCVY; > + xfs_buf_ioend_finish(bp); For someone like me who does not know all the assumptions about buffers, why is this fag leak prevention needed for log recovery buffers and not for inode/dquote buffers? Wouldn't it be better to have: bp->b_flags &= ~_XBF_BUFFER_TYPE_MASK; inside xfs_buf_ioend_finish()? For not changing logic by rearranging code: Reviewed-by: Amir Goldstein <amir73il@gmail.com> Thanks, Amir.
On Fri, May 22, 2020 at 01:50:10PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Log recovery has it's own buffer write completion handler for > buffers that it directly recovers. Convert these to direct calls by > flagging these buffers as being log recovery buffers. The flag will > get cleared by the log recovery IO completion routine, so it will > never leak out of log recovery. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/xfs_buf.c | 10 ++++++++++ > fs/xfs/xfs_buf.h | 2 ++ > fs/xfs/xfs_buf_item_recover.c | 5 ++--- > fs/xfs/xfs_dquot_item_recover.c | 2 +- > fs/xfs/xfs_inode_item_recover.c | 2 +- > fs/xfs/xfs_log_recover.c | 5 ++--- > 6 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 77d40eb4a11db..b89685ce8519d 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -14,6 +14,7 @@ > #include "xfs_mount.h" > #include "xfs_trace.h" > #include "xfs_log.h" > +#include "xfs_log_recover.h" > #include "xfs_trans.h" > #include "xfs_buf_item.h" > #include "xfs_errortag.h" > @@ -1207,6 +1208,15 @@ xfs_buf_ioend( > if (read) > goto out_finish; > > + /* > + * 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) { > + xlog_recover_iodone(bp); > + return; > + } > + > /* inodes always have a callback on write */ > if (bp->b_flags & _XBF_INODES) { > xfs_buf_inode_iodone(bp); > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h > index cbde44ecb3963..c5fe4c48c9080 100644 > --- a/fs/xfs/xfs_buf.h > +++ b/fs/xfs/xfs_buf.h > @@ -33,6 +33,7 @@ > /* 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 */ > > /* flags used only internally */ > #define _XBF_PAGES (1 << 20)/* backed by refcounted pages */ > @@ -57,6 +58,7 @@ typedef unsigned int xfs_buf_flags_t; > { XBF_WRITE_FAIL, "WRITE_FAIL" }, \ > { _XBF_INODES, "INODES" }, \ > { _XBF_DQUOTS, "DQUOTS" }, \ > + { _XBF_LOGRCVY, "LOG_RECOVERY" }, \ Pat Sajak would be very displeased with you for cheating him out of the purchase of three vowels. :P _XBF_LOGRECOVERY, please... Otherwise this looks like a pretty straightforward change. Ahah, I guess I was wrong, there are *four* states. --D > { _XBF_PAGES, "PAGES" }, \ > { _XBF_KMEM, "KMEM" }, \ > { _XBF_DELWRI_Q, "DELWRI_Q" }, \ > diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c > index 04faa7310c4f0..bfd50daa16606 100644 > --- a/fs/xfs/xfs_buf_item_recover.c > +++ b/fs/xfs/xfs_buf_item_recover.c > @@ -419,8 +419,7 @@ xlog_recover_validate_buf_type( > if (bp->b_ops) { > struct xfs_buf_log_item *bip; > > - ASSERT(!bp->b_iodone || bp->b_iodone == xlog_recover_iodone); > - bp->b_iodone = xlog_recover_iodone; > + bp->b_flags |= _XBF_LOGRCVY; > xfs_buf_item_init(bp, mp); > bip = bp->b_log_item; > bip->bli_item.li_lsn = current_lsn; > @@ -963,7 +962,7 @@ xlog_recover_buf_commit_pass2( > error = xfs_bwrite(bp); > } else { > ASSERT(bp->b_mount == mp); > - bp->b_iodone = xlog_recover_iodone; > + bp->b_flags |= _XBF_LOGRCVY; > xfs_buf_delwri_queue(bp, buffer_list); > } > > diff --git a/fs/xfs/xfs_dquot_item_recover.c b/fs/xfs/xfs_dquot_item_recover.c > index 3400be4c88f08..a0a4b089e0cdd 100644 > --- a/fs/xfs/xfs_dquot_item_recover.c > +++ b/fs/xfs/xfs_dquot_item_recover.c > @@ -153,7 +153,7 @@ xlog_recover_dquot_commit_pass2( > > ASSERT(dq_f->qlf_size == 2); > ASSERT(bp->b_mount == mp); > - bp->b_iodone = xlog_recover_iodone; > + bp->b_flags |= _XBF_LOGRCVY; > xfs_buf_delwri_queue(bp, buffer_list); > > out_release: > diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c > index dc3e26ff16c90..b67f1b7c5b65f 100644 > --- a/fs/xfs/xfs_inode_item_recover.c > +++ b/fs/xfs/xfs_inode_item_recover.c > @@ -376,7 +376,7 @@ xlog_recover_inode_commit_pass2( > xfs_dinode_calc_crc(log->l_mp, dip); > > ASSERT(bp->b_mount == mp); > - bp->b_iodone = xlog_recover_iodone; > + bp->b_flags |= _XBF_LOGRCVY; > xfs_buf_delwri_queue(bp, buffer_list); > > out_release: > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index ec015df55b77a..0aa823aeafca9 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -287,9 +287,8 @@ xlog_recover_iodone( > if (bp->b_log_item) > xfs_buf_item_relse(bp); > ASSERT(bp->b_log_item == NULL); > - > - bp->b_iodone = NULL; > - xfs_buf_ioend(bp); > + bp->b_flags &= ~_XBF_LOGRCVY; > + xfs_buf_ioend_finish(bp); > } > > /* > -- > 2.26.2.761.g0e0b3e54be >
On Fri, May 22, 2020 at 10:41:21AM +0300, Amir Goldstein wrote: > On Fri, May 22, 2020 at 6:51 AM Dave Chinner <david@fromorbit.com> wrote: > > > > From: Dave Chinner <dchinner@redhat.com> > > > > Log recovery has it's own buffer write completion handler for > > buffers that it directly recovers. Convert these to direct calls by > > flagging these buffers as being log recovery buffers. The flag will > > get cleared by the log recovery IO completion routine, so it will > > never leak out of log recovery. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > > fs/xfs/xfs_buf.c | 10 ++++++++++ > > fs/xfs/xfs_buf.h | 2 ++ > > fs/xfs/xfs_buf_item_recover.c | 5 ++--- > > fs/xfs/xfs_dquot_item_recover.c | 2 +- > > fs/xfs/xfs_inode_item_recover.c | 2 +- > > fs/xfs/xfs_log_recover.c | 5 ++--- > > 6 files changed, 18 insertions(+), 8 deletions(-) > > > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > > index 77d40eb4a11db..b89685ce8519d 100644 > > --- a/fs/xfs/xfs_buf.c > > +++ b/fs/xfs/xfs_buf.c > > @@ -14,6 +14,7 @@ > > #include "xfs_mount.h" > > #include "xfs_trace.h" > > #include "xfs_log.h" > > +#include "xfs_log_recover.h" > > #include "xfs_trans.h" > > #include "xfs_buf_item.h" > > #include "xfs_errortag.h" > > @@ -1207,6 +1208,15 @@ xfs_buf_ioend( > > if (read) > > goto out_finish; > > > > + /* > > + * 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) { > > + xlog_recover_iodone(bp); > > + return; > > + } > > + > > /* inodes always have a callback on write */ > > if (bp->b_flags & _XBF_INODES) { > > xfs_buf_inode_iodone(bp); > > This turns out to be a "static calls" pattern. Yes, that was one of the intents of this set of changes - to remove a function pointer that is largely unnecessary. The main point of it was to clearly separate out the different completions that need to be run. > I think it would look nicer as a switch statement on > (bp->b_flags & _XBF_BUFFER_TYPE_MASK) > It would be also nicer to document near flag definition > that the type flags are mutually exclusive. That doesn't actually make the code any more compact. case _XBF_LOGRCVY: /* * If this is a log recovery buffer, we aren't doing * transactional IO yet so we need to let it handle IO * completions. */ xlog_recover_iodone(bp); return; Is actually more lines of additional code than just a set of if() based flag checks. So adding a mask just to make this handling a switch statement is largely marginal as to it's benefit here. > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > > index ec015df55b77a..0aa823aeafca9 100644 > > --- a/fs/xfs/xfs_log_recover.c > > +++ b/fs/xfs/xfs_log_recover.c > > @@ -287,9 +287,8 @@ xlog_recover_iodone( > > if (bp->b_log_item) > > xfs_buf_item_relse(bp); > > ASSERT(bp->b_log_item == NULL); > > - > > - bp->b_iodone = NULL; > > - xfs_buf_ioend(bp); > > + bp->b_flags &= ~_XBF_LOGRCVY; > > + xfs_buf_ioend_finish(bp); > > > For someone like me who does not know all the assumptions > about buffers, why is this fag leak prevention needed for log recovery > buffers and not for inode/dquote buffers? > > Wouldn't it be better to have: > bp->b_flags &= ~_XBF_BUFFER_TYPE_MASK; > > inside xfs_buf_ioend_finish()? No. The flag indicates there is a completion to run on the buffer; having the completion run does not mean the next time the buffer is written the completion doesn't need to run. i.e. for writes during log recovery, the flag is a one-shot. It will be set before each write, and cleared afterwards. For something like inode cluster buffers, it will be set when the inode is attached to the buffer, and cleared when there are no more inodes attached to it. There may be hundreds of IO completions run on the buffer between attach and detatch, so clearing the _XBF_INODES flag in xfs_buf_ioend_finish() is completely wrong. i.e. the flag is not managed by the buffer cache infrastructure; it's owned by the subsystem that is attaching things to the buffer... Cheers, Dave.
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 77d40eb4a11db..b89685ce8519d 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -14,6 +14,7 @@ #include "xfs_mount.h" #include "xfs_trace.h" #include "xfs_log.h" +#include "xfs_log_recover.h" #include "xfs_trans.h" #include "xfs_buf_item.h" #include "xfs_errortag.h" @@ -1207,6 +1208,15 @@ xfs_buf_ioend( if (read) goto out_finish; + /* + * 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) { + xlog_recover_iodone(bp); + return; + } + /* inodes always have a callback on write */ if (bp->b_flags & _XBF_INODES) { xfs_buf_inode_iodone(bp); diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index cbde44ecb3963..c5fe4c48c9080 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -33,6 +33,7 @@ /* 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 */ /* flags used only internally */ #define _XBF_PAGES (1 << 20)/* backed by refcounted pages */ @@ -57,6 +58,7 @@ typedef unsigned int xfs_buf_flags_t; { XBF_WRITE_FAIL, "WRITE_FAIL" }, \ { _XBF_INODES, "INODES" }, \ { _XBF_DQUOTS, "DQUOTS" }, \ + { _XBF_LOGRCVY, "LOG_RECOVERY" }, \ { _XBF_PAGES, "PAGES" }, \ { _XBF_KMEM, "KMEM" }, \ { _XBF_DELWRI_Q, "DELWRI_Q" }, \ diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c index 04faa7310c4f0..bfd50daa16606 100644 --- a/fs/xfs/xfs_buf_item_recover.c +++ b/fs/xfs/xfs_buf_item_recover.c @@ -419,8 +419,7 @@ xlog_recover_validate_buf_type( if (bp->b_ops) { struct xfs_buf_log_item *bip; - ASSERT(!bp->b_iodone || bp->b_iodone == xlog_recover_iodone); - bp->b_iodone = xlog_recover_iodone; + bp->b_flags |= _XBF_LOGRCVY; xfs_buf_item_init(bp, mp); bip = bp->b_log_item; bip->bli_item.li_lsn = current_lsn; @@ -963,7 +962,7 @@ xlog_recover_buf_commit_pass2( error = xfs_bwrite(bp); } else { ASSERT(bp->b_mount == mp); - bp->b_iodone = xlog_recover_iodone; + bp->b_flags |= _XBF_LOGRCVY; xfs_buf_delwri_queue(bp, buffer_list); } diff --git a/fs/xfs/xfs_dquot_item_recover.c b/fs/xfs/xfs_dquot_item_recover.c index 3400be4c88f08..a0a4b089e0cdd 100644 --- a/fs/xfs/xfs_dquot_item_recover.c +++ b/fs/xfs/xfs_dquot_item_recover.c @@ -153,7 +153,7 @@ xlog_recover_dquot_commit_pass2( ASSERT(dq_f->qlf_size == 2); ASSERT(bp->b_mount == mp); - bp->b_iodone = xlog_recover_iodone; + bp->b_flags |= _XBF_LOGRCVY; xfs_buf_delwri_queue(bp, buffer_list); out_release: diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c index dc3e26ff16c90..b67f1b7c5b65f 100644 --- a/fs/xfs/xfs_inode_item_recover.c +++ b/fs/xfs/xfs_inode_item_recover.c @@ -376,7 +376,7 @@ xlog_recover_inode_commit_pass2( xfs_dinode_calc_crc(log->l_mp, dip); ASSERT(bp->b_mount == mp); - bp->b_iodone = xlog_recover_iodone; + bp->b_flags |= _XBF_LOGRCVY; xfs_buf_delwri_queue(bp, buffer_list); out_release: diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index ec015df55b77a..0aa823aeafca9 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -287,9 +287,8 @@ xlog_recover_iodone( if (bp->b_log_item) xfs_buf_item_relse(bp); ASSERT(bp->b_log_item == NULL); - - bp->b_iodone = NULL; - xfs_buf_ioend(bp); + bp->b_flags &= ~_XBF_LOGRCVY; + xfs_buf_ioend_finish(bp); } /*