* [PATCH 0/3] Minor cleanups
@ 2019-08-13 9:03 Nikolay Borisov
2019-08-13 9:03 ` [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere Nikolay Borisov
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Nikolay Borisov @ 2019-08-13 9:03 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, Nikolay Borisov
While digging around xfs' buf code I spotted a couple of cleanup candidates
which resulted in this patch. It should hopefully make the code easier to inspect
by reducing the 'hops' to the actual implementation of buffer submission.
Nikolay Borisov (3):
xfs: Use __xfs_buf_submit everywhere
xfs: Rename __xfs_buf_submit to xfs_buf_submit
xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP
fs/xfs/xfs_buf.c | 16 +++++++++-------
fs/xfs/xfs_buf.h | 16 ++++------------
fs/xfs/xfs_buf_item.c | 2 +-
fs/xfs/xfs_log_recover.c | 2 +-
fs/xfs/xfs_trans.h | 6 ++++--
5 files changed, 19 insertions(+), 23 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere 2019-08-13 9:03 [PATCH 0/3] Minor cleanups Nikolay Borisov @ 2019-08-13 9:03 ` Nikolay Borisov 2019-08-13 11:55 ` Brian Foster 2019-08-13 9:03 ` [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit Nikolay Borisov 2019-08-13 9:03 ` [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP Nikolay Borisov 2 siblings, 1 reply; 11+ messages in thread From: Nikolay Borisov @ 2019-08-13 9:03 UTC (permalink / raw) To: linux-xfs; +Cc: darrick.wong, Nikolay Borisov Currently xfs_buf_submit is used as a tiny wrapper to __xfs_buf_submit. It only checks whether XFB_ASYNC flag is set and sets the second parameter to __xfs_buf_submit accordingly. It's possible to remove the level of indirection since in all contexts where xfs_buf_submit is called we already know if XBF_ASYNC is set or not. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/xfs/xfs_buf.c | 8 +++++--- fs/xfs/xfs_buf_item.c | 2 +- fs/xfs/xfs_log_recover.c | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index ca0849043f54..a75d05e49a98 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -751,13 +751,15 @@ _xfs_buf_read( xfs_buf_t *bp, xfs_buf_flags_t flags) { + bool wait = bp->b_flags & XBF_ASYNC ? false : true; + ASSERT(!(flags & XBF_WRITE)); ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL); bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD); bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD); - return xfs_buf_submit(bp); + return __xfs_buf_submit(bp, wait); } /* @@ -883,7 +885,7 @@ xfs_buf_read_uncached( bp->b_flags |= XBF_READ; bp->b_ops = ops; - xfs_buf_submit(bp); + __xfs_buf_submit(bp, true); if (bp->b_error) { int error = bp->b_error; xfs_buf_relse(bp); @@ -1214,7 +1216,7 @@ xfs_bwrite( bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q | XBF_WRITE_FAIL | XBF_DONE); - error = xfs_buf_submit(bp); + error = __xfs_buf_submit(bp, true); if (error) xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); return error; diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c index 7dcaec54a20b..fef08980dd21 100644 --- a/fs/xfs/xfs_buf_item.c +++ b/fs/xfs/xfs_buf_item.c @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error( bp->b_first_retry_time = jiffies; xfs_buf_ioerror(bp, 0); - xfs_buf_submit(bp); + __xfs_buf_submit(bp, false); return true; } diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 13d1d3e95b88..64e315f80147 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -5610,7 +5610,7 @@ xlog_do_recover( bp->b_flags |= XBF_READ; bp->b_ops = &xfs_sb_buf_ops; - error = xfs_buf_submit(bp); + error = __xfs_buf_submit(bp, true); if (error) { if (!XFS_FORCED_SHUTDOWN(mp)) { xfs_buf_ioerror_alert(bp, __func__); -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere 2019-08-13 9:03 ` [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere Nikolay Borisov @ 2019-08-13 11:55 ` Brian Foster 2019-08-13 12:06 ` Nikolay Borisov 0 siblings, 1 reply; 11+ messages in thread From: Brian Foster @ 2019-08-13 11:55 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-xfs, darrick.wong On Tue, Aug 13, 2019 at 12:03:04PM +0300, Nikolay Borisov wrote: > Currently xfs_buf_submit is used as a tiny wrapper to __xfs_buf_submit. > It only checks whether XFB_ASYNC flag is set and sets the second > parameter to __xfs_buf_submit accordingly. It's possible to remove the > level of indirection since in all contexts where xfs_buf_submit is > called we already know if XBF_ASYNC is set or not. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- Random nit: the use of upper case in the first word of the commit log subject line kind of stands out to me. I know there are other instances of this (I think I noticed one the other day), but my presumption was that it was random/accidental where your patches seem to do it intentionally. Do we have a common practice here? Do we care? I prefer consistency of using lower case for normal text, but it's really just a nit. > fs/xfs/xfs_buf.c | 8 +++++--- > fs/xfs/xfs_buf_item.c | 2 +- > fs/xfs/xfs_log_recover.c | 2 +- > 3 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index ca0849043f54..a75d05e49a98 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -751,13 +751,15 @@ _xfs_buf_read( > xfs_buf_t *bp, > xfs_buf_flags_t flags) > { > + bool wait = bp->b_flags & XBF_ASYNC ? false : true; > + This doesn't look quite right. Just below we clear several flags from ->b_flags then potentially reapply based on the flags parameter. Hence, I think ->b_flags above may not reflect ->b_flags by the time we call __xfs_buf_submit(). Brian > ASSERT(!(flags & XBF_WRITE)); > ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL); > > bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD); > bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD); > > - return xfs_buf_submit(bp); > + return __xfs_buf_submit(bp, wait); > } > > /* > @@ -883,7 +885,7 @@ xfs_buf_read_uncached( > bp->b_flags |= XBF_READ; > bp->b_ops = ops; > > - xfs_buf_submit(bp); > + __xfs_buf_submit(bp, true); > if (bp->b_error) { > int error = bp->b_error; > xfs_buf_relse(bp); > @@ -1214,7 +1216,7 @@ xfs_bwrite( > bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q | > XBF_WRITE_FAIL | XBF_DONE); > > - error = xfs_buf_submit(bp); > + error = __xfs_buf_submit(bp, true); > if (error) > xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); > return error; > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c > index 7dcaec54a20b..fef08980dd21 100644 > --- a/fs/xfs/xfs_buf_item.c > +++ b/fs/xfs/xfs_buf_item.c > @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error( > bp->b_first_retry_time = jiffies; > > xfs_buf_ioerror(bp, 0); > - xfs_buf_submit(bp); > + __xfs_buf_submit(bp, false); > return true; > } > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 13d1d3e95b88..64e315f80147 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -5610,7 +5610,7 @@ xlog_do_recover( > bp->b_flags |= XBF_READ; > bp->b_ops = &xfs_sb_buf_ops; > > - error = xfs_buf_submit(bp); > + error = __xfs_buf_submit(bp, true); > if (error) { > if (!XFS_FORCED_SHUTDOWN(mp)) { > xfs_buf_ioerror_alert(bp, __func__); > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere 2019-08-13 11:55 ` Brian Foster @ 2019-08-13 12:06 ` Nikolay Borisov 2019-08-13 12:15 ` Nikolay Borisov 0 siblings, 1 reply; 11+ messages in thread From: Nikolay Borisov @ 2019-08-13 12:06 UTC (permalink / raw) To: Brian Foster; +Cc: darrick.wong, linux-xfs On 13.08.19 г. 14:55 ч., Brian Foster wrote: > On Tue, Aug 13, 2019 at 12:03:04PM +0300, Nikolay Borisov wrote: >> Currently xfs_buf_submit is used as a tiny wrapper to __xfs_buf_submit. >> It only checks whether XFB_ASYNC flag is set and sets the second >> parameter to __xfs_buf_submit accordingly. It's possible to remove the >> level of indirection since in all contexts where xfs_buf_submit is >> called we already know if XBF_ASYNC is set or not. >> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >> --- > > Random nit: the use of upper case in the first word of the commit log > subject line kind of stands out to me. I know there are other instances > of this (I think I noticed one the other day), but my presumption was > that it was random/accidental where your patches seem to do it > intentionally. Do we have a common practice here? Do we care? I prefer > consistency of using lower case for normal text, but it's really just a > nit. I consider the commit log subject and commit log body to be 2 separate paragraphs, hence I start each one with capital letter. > >> fs/xfs/xfs_buf.c | 8 +++++--- >> fs/xfs/xfs_buf_item.c | 2 +- >> fs/xfs/xfs_log_recover.c | 2 +- >> 3 files changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c >> index ca0849043f54..a75d05e49a98 100644 >> --- a/fs/xfs/xfs_buf.c >> +++ b/fs/xfs/xfs_buf.c >> @@ -751,13 +751,15 @@ _xfs_buf_read( >> xfs_buf_t *bp, >> xfs_buf_flags_t flags) >> { >> + bool wait = bp->b_flags & XBF_ASYNC ? false : true; >> + > > This doesn't look quite right. Just below we clear several flags from > ->b_flags then potentially reapply based on the flags parameter. Hence, > I think ->b_flags above may not reflect ->b_flags by the time we call > __xfs_buf_submit(). It's correct the flag clearing/setting ensures that the only flags we have in bp->b_flags are in the set: flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD); So if XBF_ASYNC was set initially it will also be set when we call xfs_buf_submit. > > Brian > >> ASSERT(!(flags & XBF_WRITE)); >> ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL); >> >> bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD); >> bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD); >> >> - return xfs_buf_submit(bp); >> + return __xfs_buf_submit(bp, wait); >> } >> >> /* >> @@ -883,7 +885,7 @@ xfs_buf_read_uncached( >> bp->b_flags |= XBF_READ; >> bp->b_ops = ops; >> >> - xfs_buf_submit(bp); >> + __xfs_buf_submit(bp, true); >> if (bp->b_error) { >> int error = bp->b_error; >> xfs_buf_relse(bp); >> @@ -1214,7 +1216,7 @@ xfs_bwrite( >> bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q | >> XBF_WRITE_FAIL | XBF_DONE); >> >> - error = xfs_buf_submit(bp); >> + error = __xfs_buf_submit(bp, true); >> if (error) >> xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); >> return error; >> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c >> index 7dcaec54a20b..fef08980dd21 100644 >> --- a/fs/xfs/xfs_buf_item.c >> +++ b/fs/xfs/xfs_buf_item.c >> @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error( >> bp->b_first_retry_time = jiffies; >> >> xfs_buf_ioerror(bp, 0); >> - xfs_buf_submit(bp); >> + __xfs_buf_submit(bp, false); >> return true; >> } >> >> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c >> index 13d1d3e95b88..64e315f80147 100644 >> --- a/fs/xfs/xfs_log_recover.c >> +++ b/fs/xfs/xfs_log_recover.c >> @@ -5610,7 +5610,7 @@ xlog_do_recover( >> bp->b_flags |= XBF_READ; >> bp->b_ops = &xfs_sb_buf_ops; >> >> - error = xfs_buf_submit(bp); >> + error = __xfs_buf_submit(bp, true); >> if (error) { >> if (!XFS_FORCED_SHUTDOWN(mp)) { >> xfs_buf_ioerror_alert(bp, __func__); >> -- >> 2.17.1 >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere 2019-08-13 12:06 ` Nikolay Borisov @ 2019-08-13 12:15 ` Nikolay Borisov 0 siblings, 0 replies; 11+ messages in thread From: Nikolay Borisov @ 2019-08-13 12:15 UTC (permalink / raw) To: Brian Foster; +Cc: darrick.wong, linux-xfs On 13.08.19 г. 15:06 ч., Nikolay Borisov wrote: > > > On 13.08.19 г. 14:55 ч., Brian Foster wrote: >> On Tue, Aug 13, 2019 at 12:03:04PM +0300, Nikolay Borisov wrote: >>> Currently xfs_buf_submit is used as a tiny wrapper to __xfs_buf_submit. >>> It only checks whether XFB_ASYNC flag is set and sets the second >>> parameter to __xfs_buf_submit accordingly. It's possible to remove the >>> level of indirection since in all contexts where xfs_buf_submit is >>> called we already know if XBF_ASYNC is set or not. >>> >>> Signed-off-by: Nikolay Borisov <nborisov@suse.com> >>> --- >> >> Random nit: the use of upper case in the first word of the commit log >> subject line kind of stands out to me. I know there are other instances >> of this (I think I noticed one the other day), but my presumption was >> that it was random/accidental where your patches seem to do it >> intentionally. Do we have a common practice here? Do we care? I prefer >> consistency of using lower case for normal text, but it's really just a >> nit. > > I consider the commit log subject and commit log body to be 2 separate > paragraphs, hence I start each one with capital letter. > >> >>> fs/xfs/xfs_buf.c | 8 +++++--- >>> fs/xfs/xfs_buf_item.c | 2 +- >>> fs/xfs/xfs_log_recover.c | 2 +- >>> 3 files changed, 7 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c >>> index ca0849043f54..a75d05e49a98 100644 >>> --- a/fs/xfs/xfs_buf.c >>> +++ b/fs/xfs/xfs_buf.c >>> @@ -751,13 +751,15 @@ _xfs_buf_read( >>> xfs_buf_t *bp, >>> xfs_buf_flags_t flags) >>> { >>> + bool wait = bp->b_flags & XBF_ASYNC ? false : true; >>> + >> >> This doesn't look quite right. Just below we clear several flags from >> ->b_flags then potentially reapply based on the flags parameter. Hence, >> I think ->b_flags above may not reflect ->b_flags by the time we call >> __xfs_buf_submit(). > > It's correct the flag clearing/setting ensures that the only flags we > have in bp->b_flags are in the set: flags & (XBF_READ | XBF_ASYNC | > XBF_READ_AHEAD); > > So if XBF_ASYNC was set initially it will also be set when we call > xfs_buf_submit. Ah, I see what you meant, indeed the correct check would be : flags & XBF_ASYNC ... I will wait to see if people actually consider this series useful and then resubmit a fixed version. > > >> >> Brian >> >>> ASSERT(!(flags & XBF_WRITE)); >>> ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL); >>> >>> bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD); >>> bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD); >>> >>> - return xfs_buf_submit(bp); >>> + return __xfs_buf_submit(bp, wait); >>> } >>> >>> /* >>> @@ -883,7 +885,7 @@ xfs_buf_read_uncached( >>> bp->b_flags |= XBF_READ; >>> bp->b_ops = ops; >>> >>> - xfs_buf_submit(bp); >>> + __xfs_buf_submit(bp, true); >>> if (bp->b_error) { >>> int error = bp->b_error; >>> xfs_buf_relse(bp); >>> @@ -1214,7 +1216,7 @@ xfs_bwrite( >>> bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q | >>> XBF_WRITE_FAIL | XBF_DONE); >>> >>> - error = xfs_buf_submit(bp); >>> + error = __xfs_buf_submit(bp, true); >>> if (error) >>> xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); >>> return error; >>> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c >>> index 7dcaec54a20b..fef08980dd21 100644 >>> --- a/fs/xfs/xfs_buf_item.c >>> +++ b/fs/xfs/xfs_buf_item.c >>> @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error( >>> bp->b_first_retry_time = jiffies; >>> >>> xfs_buf_ioerror(bp, 0); >>> - xfs_buf_submit(bp); >>> + __xfs_buf_submit(bp, false); >>> return true; >>> } >>> >>> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c >>> index 13d1d3e95b88..64e315f80147 100644 >>> --- a/fs/xfs/xfs_log_recover.c >>> +++ b/fs/xfs/xfs_log_recover.c >>> @@ -5610,7 +5610,7 @@ xlog_do_recover( >>> bp->b_flags |= XBF_READ; >>> bp->b_ops = &xfs_sb_buf_ops; >>> >>> - error = xfs_buf_submit(bp); >>> + error = __xfs_buf_submit(bp, true); >>> if (error) { >>> if (!XFS_FORCED_SHUTDOWN(mp)) { >>> xfs_buf_ioerror_alert(bp, __func__); >>> -- >>> 2.17.1 >>> >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit 2019-08-13 9:03 [PATCH 0/3] Minor cleanups Nikolay Borisov 2019-08-13 9:03 ` [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere Nikolay Borisov @ 2019-08-13 9:03 ` Nikolay Borisov 2019-08-13 11:56 ` Brian Foster 2019-08-13 9:03 ` [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP Nikolay Borisov 2 siblings, 1 reply; 11+ messages in thread From: Nikolay Borisov @ 2019-08-13 9:03 UTC (permalink / raw) To: linux-xfs; +Cc: darrick.wong, Nikolay Borisov Since xfs_buf_submit no longer has any callers just rename its __ prefixed counterpart. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/xfs/xfs_buf.c | 10 +++++----- fs/xfs/xfs_buf.h | 7 +------ fs/xfs/xfs_buf_item.c | 2 +- fs/xfs/xfs_log_recover.c | 2 +- 4 files changed, 8 insertions(+), 13 deletions(-) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index a75d05e49a98..99c66f80d7cc 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -759,7 +759,7 @@ _xfs_buf_read( bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD); bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD); - return __xfs_buf_submit(bp, wait); + return xfs_buf_submit(bp, wait); } /* @@ -885,7 +885,7 @@ xfs_buf_read_uncached( bp->b_flags |= XBF_READ; bp->b_ops = ops; - __xfs_buf_submit(bp, true); + xfs_buf_submit(bp, true); if (bp->b_error) { int error = bp->b_error; xfs_buf_relse(bp); @@ -1216,7 +1216,7 @@ xfs_bwrite( bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q | XBF_WRITE_FAIL | XBF_DONE); - error = __xfs_buf_submit(bp, true); + error = xfs_buf_submit(bp, true); if (error) xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); return error; @@ -1427,7 +1427,7 @@ xfs_buf_iowait( * holds an additional reference itself. */ int -__xfs_buf_submit( +xfs_buf_submit( struct xfs_buf *bp, bool wait) { @@ -1929,7 +1929,7 @@ xfs_buf_delwri_submit_buffers( bp->b_flags |= XBF_ASYNC; list_del_init(&bp->b_list); } - __xfs_buf_submit(bp, false); + xfs_buf_submit(bp, false); } blk_finish_plug(&plug); diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index c6e57a3f409e..ec7037284d62 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -262,12 +262,7 @@ extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error, #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address) extern void xfs_buf_ioerror_alert(struct xfs_buf *, const char *func); -extern int __xfs_buf_submit(struct xfs_buf *bp, bool); -static inline int xfs_buf_submit(struct xfs_buf *bp) -{ - bool wait = bp->b_flags & XBF_ASYNC ? false : true; - return __xfs_buf_submit(bp, wait); -} +extern int xfs_buf_submit(struct xfs_buf *bp, bool); void xfs_buf_zero(struct xfs_buf *bp, size_t boff, size_t bsize); diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c index fef08980dd21..93f38fdceb80 100644 --- a/fs/xfs/xfs_buf_item.c +++ b/fs/xfs/xfs_buf_item.c @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error( bp->b_first_retry_time = jiffies; xfs_buf_ioerror(bp, 0); - __xfs_buf_submit(bp, false); + xfs_buf_submit(bp, false); return true; } diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c index 64e315f80147..9b7822638f83 100644 --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -5610,7 +5610,7 @@ xlog_do_recover( bp->b_flags |= XBF_READ; bp->b_ops = &xfs_sb_buf_ops; - error = __xfs_buf_submit(bp, true); + error = xfs_buf_submit(bp, true); if (error) { if (!XFS_FORCED_SHUTDOWN(mp)) { xfs_buf_ioerror_alert(bp, __func__); -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit 2019-08-13 9:03 ` [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit Nikolay Borisov @ 2019-08-13 11:56 ` Brian Foster 2019-08-14 10:14 ` Dave Chinner 0 siblings, 1 reply; 11+ messages in thread From: Brian Foster @ 2019-08-13 11:56 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-xfs, darrick.wong On Tue, Aug 13, 2019 at 12:03:05PM +0300, Nikolay Borisov wrote: > Since xfs_buf_submit no longer has any callers just rename its __ > prefixed counterpart. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- Now we have a primary submission interface that allows combinations of XBF_ASYNC and waiting or not while the underlying mechanisms are not so flexible. It looks like the current factoring exists to support delwri queues where we never wait in buffer submission regardless of async state because we are batching the submission/wait across multiple buffers. But what happens if a caller passes an async buffer with wait == true? I/O completion only completes ->b_iowait if XBF_ASYNC is clear. I find this rather confusing because now a caller needs to know about implementation details to use the function properly. That's already true of __xfs_buf_submit(), but that's partly why it's named as an "internal" function. I think we ultimately need the interface flexibility so the delwri case can continue to work. One option could be to update xfs_buf_submit() such that we never wait on an XBF_ASYNC buffer and add an assert to flag wait == true as invalid, but TBH I'm not convinced this is any simpler than the current interface where most callers simply only need to care about the flag. Maybe others have thoughts... Brian > fs/xfs/xfs_buf.c | 10 +++++----- > fs/xfs/xfs_buf.h | 7 +------ > fs/xfs/xfs_buf_item.c | 2 +- > fs/xfs/xfs_log_recover.c | 2 +- > 4 files changed, 8 insertions(+), 13 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index a75d05e49a98..99c66f80d7cc 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -759,7 +759,7 @@ _xfs_buf_read( > bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD); > bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD); > > - return __xfs_buf_submit(bp, wait); > + return xfs_buf_submit(bp, wait); > } > > /* > @@ -885,7 +885,7 @@ xfs_buf_read_uncached( > bp->b_flags |= XBF_READ; > bp->b_ops = ops; > > - __xfs_buf_submit(bp, true); > + xfs_buf_submit(bp, true); > if (bp->b_error) { > int error = bp->b_error; > xfs_buf_relse(bp); > @@ -1216,7 +1216,7 @@ xfs_bwrite( > bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q | > XBF_WRITE_FAIL | XBF_DONE); > > - error = __xfs_buf_submit(bp, true); > + error = xfs_buf_submit(bp, true); > if (error) > xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR); > return error; > @@ -1427,7 +1427,7 @@ xfs_buf_iowait( > * holds an additional reference itself. > */ > int > -__xfs_buf_submit( > +xfs_buf_submit( > struct xfs_buf *bp, > bool wait) > { > @@ -1929,7 +1929,7 @@ xfs_buf_delwri_submit_buffers( > bp->b_flags |= XBF_ASYNC; > list_del_init(&bp->b_list); > } > - __xfs_buf_submit(bp, false); > + xfs_buf_submit(bp, false); > } > blk_finish_plug(&plug); > > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h > index c6e57a3f409e..ec7037284d62 100644 > --- a/fs/xfs/xfs_buf.h > +++ b/fs/xfs/xfs_buf.h > @@ -262,12 +262,7 @@ extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error, > #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address) > extern void xfs_buf_ioerror_alert(struct xfs_buf *, const char *func); > > -extern int __xfs_buf_submit(struct xfs_buf *bp, bool); > -static inline int xfs_buf_submit(struct xfs_buf *bp) > -{ > - bool wait = bp->b_flags & XBF_ASYNC ? false : true; > - return __xfs_buf_submit(bp, wait); > -} > +extern int xfs_buf_submit(struct xfs_buf *bp, bool); > > void xfs_buf_zero(struct xfs_buf *bp, size_t boff, size_t bsize); > > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c > index fef08980dd21..93f38fdceb80 100644 > --- a/fs/xfs/xfs_buf_item.c > +++ b/fs/xfs/xfs_buf_item.c > @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error( > bp->b_first_retry_time = jiffies; > > xfs_buf_ioerror(bp, 0); > - __xfs_buf_submit(bp, false); > + xfs_buf_submit(bp, false); > return true; > } > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index 64e315f80147..9b7822638f83 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -5610,7 +5610,7 @@ xlog_do_recover( > bp->b_flags |= XBF_READ; > bp->b_ops = &xfs_sb_buf_ops; > > - error = __xfs_buf_submit(bp, true); > + error = xfs_buf_submit(bp, true); > if (error) { > if (!XFS_FORCED_SHUTDOWN(mp)) { > xfs_buf_ioerror_alert(bp, __func__); > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit 2019-08-13 11:56 ` Brian Foster @ 2019-08-14 10:14 ` Dave Chinner 0 siblings, 0 replies; 11+ messages in thread From: Dave Chinner @ 2019-08-14 10:14 UTC (permalink / raw) To: Brian Foster; +Cc: Nikolay Borisov, linux-xfs, darrick.wong On Tue, Aug 13, 2019 at 07:56:58AM -0400, Brian Foster wrote: > On Tue, Aug 13, 2019 at 12:03:05PM +0300, Nikolay Borisov wrote: > > Since xfs_buf_submit no longer has any callers just rename its __ > > prefixed counterpart. > > > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > > --- > > Now we have a primary submission interface that allows combinations of > XBF_ASYNC and waiting or not while the underlying mechanisms are not so > flexible. It looks like the current factoring exists to support delwri > queues where we never wait in buffer submission regardless of async > state because we are batching the submission/wait across multiple > buffers. But what happens if a caller passes an async buffer with wait > == true? I/O completion only completes ->b_iowait if XBF_ASYNC is clear. > > I find this rather confusing because now a caller needs to know about > implementation details to use the function properly. That's already true > of __xfs_buf_submit(), but that's partly why it's named as an "internal" > function. I think we ultimately need the interface flexibility so the > delwri case can continue to work. One option could be to update > xfs_buf_submit() such that we never wait on an XBF_ASYNC buffer and add > an assert to flag wait == true as invalid, but TBH I'm not convinced > this is any simpler than the current interface where most callers simply > only need to care about the flag. Maybe others have thoughts... Yeah, we slpit the code u plike this intentionally to separate out the different ways of submitting IO so that we didn't end up using invalid methods, like ASYNC + wait, which would lead to hangs waiting for IO that has already completed. I much prefer the code as it stands now - it may be slightly more verbose, but it's simple to understand and hard to use incorrectly.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP 2019-08-13 9:03 [PATCH 0/3] Minor cleanups Nikolay Borisov 2019-08-13 9:03 ` [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere Nikolay Borisov 2019-08-13 9:03 ` [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit Nikolay Borisov @ 2019-08-13 9:03 ` Nikolay Borisov 2019-08-13 11:57 ` Brian Foster 2019-08-14 10:23 ` Dave Chinner 2 siblings, 2 replies; 11+ messages in thread From: Nikolay Borisov @ 2019-08-13 9:03 UTC (permalink / raw) To: linux-xfs; +Cc: darrick.wong, Nikolay Borisov This macro encodes a trivial struct initializations, just open code it. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/xfs/xfs_buf.c | 4 ++-- fs/xfs/xfs_buf.h | 9 +++------ fs/xfs/xfs_trans.h | 6 ++++-- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 99c66f80d7cc..389c5b590f11 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -658,7 +658,7 @@ xfs_buf_incore( { struct xfs_buf *bp; int error; - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; error = xfs_buf_find(target, &map, 1, flags, NULL, &bp); if (error) @@ -905,7 +905,7 @@ xfs_buf_get_uncached( unsigned long page_count; int error, i; struct xfs_buf *bp; - DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks); + struct xfs_buf_map map = { .bm_bn = XFS_BUF_DADDR_NULL, .bm_len = numblks }; /* flags might contain irrelevant bits, pass only what we care about */ bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT); diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h index ec7037284d62..548dfb0c6e27 100644 --- a/fs/xfs/xfs_buf.h +++ b/fs/xfs/xfs_buf.h @@ -104,9 +104,6 @@ struct xfs_buf_map { int bm_len; /* size of I/O */ }; -#define DEFINE_SINGLE_BUF_MAP(map, blkno, numblk) \ - struct xfs_buf_map (map) = { .bm_bn = (blkno), .bm_len = (numblk) }; - struct xfs_buf_ops { char *name; union { @@ -209,7 +206,7 @@ xfs_buf_get( xfs_daddr_t blkno, size_t numblks) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; return xfs_buf_get_map(target, &map, 1, 0); } @@ -221,7 +218,7 @@ xfs_buf_read( xfs_buf_flags_t flags, const struct xfs_buf_ops *ops) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; return xfs_buf_read_map(target, &map, 1, flags, ops); } @@ -232,7 +229,7 @@ xfs_buf_readahead( size_t numblks, const struct xfs_buf_ops *ops) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; return xfs_buf_readahead_map(target, &map, 1, ops); } diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h index 64d7f171ebd3..8d6fce5c0320 100644 --- a/fs/xfs/xfs_trans.h +++ b/fs/xfs/xfs_trans.h @@ -182,7 +182,8 @@ xfs_trans_get_buf( int numblks, uint flags) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; + return xfs_trans_get_buf_map(tp, target, &map, 1, flags); } @@ -205,7 +206,8 @@ xfs_trans_read_buf( struct xfs_buf **bpp, const struct xfs_buf_ops *ops) { - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; + return xfs_trans_read_buf_map(mp, tp, target, &map, 1, flags, bpp, ops); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP 2019-08-13 9:03 ` [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP Nikolay Borisov @ 2019-08-13 11:57 ` Brian Foster 2019-08-14 10:23 ` Dave Chinner 1 sibling, 0 replies; 11+ messages in thread From: Brian Foster @ 2019-08-13 11:57 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-xfs, darrick.wong On Tue, Aug 13, 2019 at 12:03:06PM +0300, Nikolay Borisov wrote: > This macro encodes a trivial struct initializations, just open code it. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- Seems fine: Reviewed-by: Brian Foster <bfoster@redhat.com> What might be more interesting is to audit the cases where nmap is always 1 and see if we can start eliminating some of this code where it isn't needed. For example, it looks xfs_buf_readahead_map() only ever uses nmap == 1. Can we pass a block/len directly there and push the map/nmap parameters further down the stack? FWIW, I also see several functions on a quick glance (xfs_dabuf_map(), xfs_buf_map_from_irec()) that take map/nmaps params, assert that nmaps == 1 yet still have iteration code for nmap > 1 cases. > fs/xfs/xfs_buf.c | 4 ++-- > fs/xfs/xfs_buf.h | 9 +++------ > fs/xfs/xfs_trans.h | 6 ++++-- > 3 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 99c66f80d7cc..389c5b590f11 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -658,7 +658,7 @@ xfs_buf_incore( > { > struct xfs_buf *bp; > int error; > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > > error = xfs_buf_find(target, &map, 1, flags, NULL, &bp); > if (error) > @@ -905,7 +905,7 @@ xfs_buf_get_uncached( > unsigned long page_count; > int error, i; > struct xfs_buf *bp; > - DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks); > + struct xfs_buf_map map = { .bm_bn = XFS_BUF_DADDR_NULL, .bm_len = numblks }; > > /* flags might contain irrelevant bits, pass only what we care about */ > bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT); > diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h > index ec7037284d62..548dfb0c6e27 100644 > --- a/fs/xfs/xfs_buf.h > +++ b/fs/xfs/xfs_buf.h > @@ -104,9 +104,6 @@ struct xfs_buf_map { > int bm_len; /* size of I/O */ > }; > > -#define DEFINE_SINGLE_BUF_MAP(map, blkno, numblk) \ > - struct xfs_buf_map (map) = { .bm_bn = (blkno), .bm_len = (numblk) }; > - > struct xfs_buf_ops { > char *name; > union { > @@ -209,7 +206,7 @@ xfs_buf_get( > xfs_daddr_t blkno, > size_t numblks) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > return xfs_buf_get_map(target, &map, 1, 0); > } > > @@ -221,7 +218,7 @@ xfs_buf_read( > xfs_buf_flags_t flags, > const struct xfs_buf_ops *ops) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > return xfs_buf_read_map(target, &map, 1, flags, ops); > } > > @@ -232,7 +229,7 @@ xfs_buf_readahead( > size_t numblks, > const struct xfs_buf_ops *ops) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > return xfs_buf_readahead_map(target, &map, 1, ops); > } > > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h > index 64d7f171ebd3..8d6fce5c0320 100644 > --- a/fs/xfs/xfs_trans.h > +++ b/fs/xfs/xfs_trans.h > @@ -182,7 +182,8 @@ xfs_trans_get_buf( > int numblks, > uint flags) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > + > return xfs_trans_get_buf_map(tp, target, &map, 1, flags); > } > > @@ -205,7 +206,8 @@ xfs_trans_read_buf( > struct xfs_buf **bpp, > const struct xfs_buf_ops *ops) > { > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; > + > return xfs_trans_read_buf_map(mp, tp, target, &map, 1, > flags, bpp, ops); > } > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP 2019-08-13 9:03 ` [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP Nikolay Borisov 2019-08-13 11:57 ` Brian Foster @ 2019-08-14 10:23 ` Dave Chinner 1 sibling, 0 replies; 11+ messages in thread From: Dave Chinner @ 2019-08-14 10:23 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-xfs, darrick.wong On Tue, Aug 13, 2019 at 12:03:06PM +0300, Nikolay Borisov wrote: > This macro encodes a trivial struct initializations, just open code it. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Hmmmm. We have defines for this sort of structure definition and initialisation all over the kernel. e.g. LIST_HEAD(), DEFINE_PER_CPU(), DEFINE_HASHTABLE(), DEFINE_SPINLOCK(), etc... And really, the intent of the define was to make it easy to get rid of all the callers of the non-map buffer interfaces by moving the map definition into the callers of xfs_buf_get, _read, etc and then enabling use to remove the non-map interfaces altogether. Hence I'd much prefer to see the xfs_buf_{get,read,readahead} and xfs_trans_buf_{get,read} wrapper functions go away than removing the define. > --- > fs/xfs/xfs_buf.c | 4 ++-- > fs/xfs/xfs_buf.h | 9 +++------ > fs/xfs/xfs_trans.h | 6 ++++-- > 3 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 99c66f80d7cc..389c5b590f11 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -658,7 +658,7 @@ xfs_buf_incore( > { > struct xfs_buf *bp; > int error; > - DEFINE_SINGLE_BUF_MAP(map, blkno, numblks); > + struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks }; FWIW, I'm not a fan of single line definitions like this because they are really hard to read. If you are converting to this form, it should be like this: struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks, }; Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-14 10:25 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-13 9:03 [PATCH 0/3] Minor cleanups Nikolay Borisov 2019-08-13 9:03 ` [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere Nikolay Borisov 2019-08-13 11:55 ` Brian Foster 2019-08-13 12:06 ` Nikolay Borisov 2019-08-13 12:15 ` Nikolay Borisov 2019-08-13 9:03 ` [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit Nikolay Borisov 2019-08-13 11:56 ` Brian Foster 2019-08-14 10:14 ` Dave Chinner 2019-08-13 9:03 ` [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP Nikolay Borisov 2019-08-13 11:57 ` Brian Foster 2019-08-14 10:23 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).