| Message ID | 20220427022259.695399-3-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: intent whiteouts | expand |
On Wed, Apr 27, 2022 at 12:22:53PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > If the first operation in a string of defer ops has no intents, > then there is no reason to commit it before running the first call > to xfs_defer_finish_one(). This allows the defer ops to be used > effectively for non-intent based operations without requiring an > unnecessary extra transaction commit when first called. > > This fixes a regression in per-attribute modification transaction > count when delayed attributes are not being used. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_defer.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c > index 0805ade2d300..66b4555bda8e 100644 > --- a/fs/xfs/libxfs/xfs_defer.c > +++ b/fs/xfs/libxfs/xfs_defer.c > @@ -186,7 +186,7 @@ static const struct xfs_defer_op_type *defer_op_types[] = { > [XFS_DEFER_OPS_TYPE_AGFL_FREE] = &xfs_agfl_free_defer_type, > }; > > -static void > +static bool > xfs_defer_create_intent( > struct xfs_trans *tp, > struct xfs_defer_pending *dfp, > @@ -197,6 +197,7 @@ xfs_defer_create_intent( > if (!dfp->dfp_intent) > dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work, > dfp->dfp_count, sort); > + return dfp->dfp_intent; Same comment as last time -- please make it more obvious that we're returning whether or not ->create_intent actually added a log item: return dfp->dfp_intent != NULL; and not returning the log intent item itself. Otherwise looks ok, so with that fixed, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > } > > /* > @@ -204,16 +205,18 @@ xfs_defer_create_intent( > * associated extents, then add the entire intake list to the end of > * the pending list. > */ > -STATIC void > +static bool > xfs_defer_create_intents( > struct xfs_trans *tp) > { > struct xfs_defer_pending *dfp; > + bool ret = false; > > list_for_each_entry(dfp, &tp->t_dfops, dfp_list) { > trace_xfs_defer_create_intent(tp->t_mountp, dfp); > - xfs_defer_create_intent(tp, dfp, true); > + ret |= xfs_defer_create_intent(tp, dfp, true); > } > + return ret; > } > > /* Abort all the intents that were committed. */ > @@ -487,7 +490,7 @@ int > xfs_defer_finish_noroll( > struct xfs_trans **tp) > { > - struct xfs_defer_pending *dfp; > + struct xfs_defer_pending *dfp = NULL; > int error = 0; > LIST_HEAD(dop_pending); > > @@ -506,17 +509,19 @@ xfs_defer_finish_noroll( > * of time that any one intent item can stick around in memory, > * pinning the log tail. > */ > - xfs_defer_create_intents(*tp); > + bool has_intents = xfs_defer_create_intents(*tp); > list_splice_init(&(*tp)->t_dfops, &dop_pending); > > - error = xfs_defer_trans_roll(tp); > - if (error) > - goto out_shutdown; > + if (has_intents || dfp) { > + error = xfs_defer_trans_roll(tp); > + if (error) > + goto out_shutdown; > > - /* Possibly relog intent items to keep the log moving. */ > - error = xfs_defer_relog(tp, &dop_pending); > - if (error) > - goto out_shutdown; > + /* Possibly relog intent items to keep the log moving. */ > + error = xfs_defer_relog(tp, &dop_pending); > + if (error) > + goto out_shutdown; > + } > > dfp = list_first_entry(&dop_pending, struct xfs_defer_pending, > dfp_list); > -- > 2.35.1 >
On Tue, Apr 26, 2022 at 08:03:30PM -0700, Darrick J. Wong wrote: > On Wed, Apr 27, 2022 at 12:22:53PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > If the first operation in a string of defer ops has no intents, > > then there is no reason to commit it before running the first call > > to xfs_defer_finish_one(). This allows the defer ops to be used > > effectively for non-intent based operations without requiring an > > unnecessary extra transaction commit when first called. > > > > This fixes a regression in per-attribute modification transaction > > count when delayed attributes are not being used. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > > fs/xfs/libxfs/xfs_defer.c | 29 +++++++++++++++++------------ > > 1 file changed, 17 insertions(+), 12 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c > > index 0805ade2d300..66b4555bda8e 100644 > > --- a/fs/xfs/libxfs/xfs_defer.c > > +++ b/fs/xfs/libxfs/xfs_defer.c > > @@ -186,7 +186,7 @@ static const struct xfs_defer_op_type *defer_op_types[] = { > > [XFS_DEFER_OPS_TYPE_AGFL_FREE] = &xfs_agfl_free_defer_type, > > }; > > > > -static void > > +static bool > > xfs_defer_create_intent( > > struct xfs_trans *tp, > > struct xfs_defer_pending *dfp, > > @@ -197,6 +197,7 @@ xfs_defer_create_intent( > > if (!dfp->dfp_intent) > > dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work, > > dfp->dfp_count, sort); > > + return dfp->dfp_intent; > > Same comment as last time -- please make it more obvious that we're > returning whether or not ->create_intent actually added a log item: > > return dfp->dfp_intent != NULL; Oh, sorry, I must have missed that. My fault! Fixed. > and not returning the log intent item itself. > > Otherwise looks ok, so with that fixed, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Thanks! -Dave.
On Wed, Apr 27, 2022 at 12:22:53PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > If the first operation in a string of defer ops has no intents, > then there is no reason to commit it before running the first call > to xfs_defer_finish_one(). This allows the defer ops to be used > effectively for non-intent based operations without requiring an > unnecessary extra transaction commit when first called. > > This fixes a regression in per-attribute modification transaction > count when delayed attributes are not being used. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/xfs/libxfs/xfs_defer.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c > index 0805ade2d300..66b4555bda8e 100644 > --- a/fs/xfs/libxfs/xfs_defer.c > +++ b/fs/xfs/libxfs/xfs_defer.c > @@ -186,7 +186,7 @@ static const struct xfs_defer_op_type *defer_op_types[] = { > [XFS_DEFER_OPS_TYPE_AGFL_FREE] = &xfs_agfl_free_defer_type, > }; > > -static void > +static bool > xfs_defer_create_intent( > struct xfs_trans *tp, > struct xfs_defer_pending *dfp, > @@ -197,6 +197,7 @@ xfs_defer_create_intent( > if (!dfp->dfp_intent) > dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work, > dfp->dfp_count, sort); > + return dfp->dfp_intent; > } > > /* > @@ -204,16 +205,18 @@ xfs_defer_create_intent( > * associated extents, then add the entire intake list to the end of > * the pending list. > */ > -STATIC void > +static bool > xfs_defer_create_intents( > struct xfs_trans *tp) > { > struct xfs_defer_pending *dfp; > + bool ret = false; > > list_for_each_entry(dfp, &tp->t_dfops, dfp_list) { > trace_xfs_defer_create_intent(tp->t_mountp, dfp); > - xfs_defer_create_intent(tp, dfp, true); > + ret |= xfs_defer_create_intent(tp, dfp, true); > } > + return ret; > } > > /* Abort all the intents that were committed. */ > @@ -487,7 +490,7 @@ int > xfs_defer_finish_noroll( > struct xfs_trans **tp) > { > - struct xfs_defer_pending *dfp; > + struct xfs_defer_pending *dfp = NULL; > int error = 0; > LIST_HEAD(dop_pending); > > @@ -506,17 +509,19 @@ xfs_defer_finish_noroll( > * of time that any one intent item can stick around in memory, > * pinning the log tail. > */ > - xfs_defer_create_intents(*tp); > + bool has_intents = xfs_defer_create_intents(*tp); > list_splice_init(&(*tp)->t_dfops, &dop_pending); While we're nitpicking: an emptry line under the variable declaration would be nice to have. > + /* Possibly relog intent items to keep the log moving. */ And a really annoying 81 character line that just wraps around here. But except for the nitpicks this looks good to me: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, 2022-04-27 at 12:22 +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > If the first operation in a string of defer ops has no intents, > then there is no reason to commit it before running the first call > to xfs_defer_finish_one(). This allows the defer ops to be used > effectively for non-intent based operations without requiring an > unnecessary extra transaction commit when first called. > > This fixes a regression in per-attribute modification transaction > count when delayed attributes are not being used. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> Just realized I forgot to send this along in my last review. Nits from other reviews aside, I think this one looks good Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_defer.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c > index 0805ade2d300..66b4555bda8e 100644 > --- a/fs/xfs/libxfs/xfs_defer.c > +++ b/fs/xfs/libxfs/xfs_defer.c > @@ -186,7 +186,7 @@ static const struct xfs_defer_op_type > *defer_op_types[] = { > [XFS_DEFER_OPS_TYPE_AGFL_FREE] = &xfs_agfl_free_defer_type, > }; > > -static void > +static bool > xfs_defer_create_intent( > struct xfs_trans *tp, > struct xfs_defer_pending *dfp, > @@ -197,6 +197,7 @@ xfs_defer_create_intent( > if (!dfp->dfp_intent) > dfp->dfp_intent = ops->create_intent(tp, &dfp- > >dfp_work, > dfp->dfp_count, > sort); > + return dfp->dfp_intent; > } > > /* > @@ -204,16 +205,18 @@ xfs_defer_create_intent( > * associated extents, then add the entire intake list to the end of > * the pending list. > */ > -STATIC void > +static bool > xfs_defer_create_intents( > struct xfs_trans *tp) > { > struct xfs_defer_pending *dfp; > + bool ret = false; > > list_for_each_entry(dfp, &tp->t_dfops, dfp_list) { > trace_xfs_defer_create_intent(tp->t_mountp, dfp); > - xfs_defer_create_intent(tp, dfp, true); > + ret |= xfs_defer_create_intent(tp, dfp, true); > } > + return ret; > } > > /* Abort all the intents that were committed. */ > @@ -487,7 +490,7 @@ int > xfs_defer_finish_noroll( > struct xfs_trans **tp) > { > - struct xfs_defer_pending *dfp; > + struct xfs_defer_pending *dfp = NULL; > int error = 0; > LIST_HEAD(dop_pending); > > @@ -506,17 +509,19 @@ xfs_defer_finish_noroll( > * of time that any one intent item can stick around in > memory, > * pinning the log tail. > */ > - xfs_defer_create_intents(*tp); > + bool has_intents = xfs_defer_create_intents(*tp); > list_splice_init(&(*tp)->t_dfops, &dop_pending); > > - error = xfs_defer_trans_roll(tp); > - if (error) > - goto out_shutdown; > + if (has_intents || dfp) { > + error = xfs_defer_trans_roll(tp); > + if (error) > + goto out_shutdown; > > - /* Possibly relog intent items to keep the log moving. > */ > - error = xfs_defer_relog(tp, &dop_pending); > - if (error) > - goto out_shutdown; > + /* Possibly relog intent items to keep the log > moving. */ > + error = xfs_defer_relog(tp, &dop_pending); > + if (error) > + goto out_shutdown; > + } > > dfp = list_first_entry(&dop_pending, struct > xfs_defer_pending, > dfp_list);
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c index 0805ade2d300..66b4555bda8e 100644 --- a/fs/xfs/libxfs/xfs_defer.c +++ b/fs/xfs/libxfs/xfs_defer.c @@ -186,7 +186,7 @@ static const struct xfs_defer_op_type *defer_op_types[] = { [XFS_DEFER_OPS_TYPE_AGFL_FREE] = &xfs_agfl_free_defer_type, }; -static void +static bool xfs_defer_create_intent( struct xfs_trans *tp, struct xfs_defer_pending *dfp, @@ -197,6 +197,7 @@ xfs_defer_create_intent( if (!dfp->dfp_intent) dfp->dfp_intent = ops->create_intent(tp, &dfp->dfp_work, dfp->dfp_count, sort); + return dfp->dfp_intent; } /* @@ -204,16 +205,18 @@ xfs_defer_create_intent( * associated extents, then add the entire intake list to the end of * the pending list. */ -STATIC void +static bool xfs_defer_create_intents( struct xfs_trans *tp) { struct xfs_defer_pending *dfp; + bool ret = false; list_for_each_entry(dfp, &tp->t_dfops, dfp_list) { trace_xfs_defer_create_intent(tp->t_mountp, dfp); - xfs_defer_create_intent(tp, dfp, true); + ret |= xfs_defer_create_intent(tp, dfp, true); } + return ret; } /* Abort all the intents that were committed. */ @@ -487,7 +490,7 @@ int xfs_defer_finish_noroll( struct xfs_trans **tp) { - struct xfs_defer_pending *dfp; + struct xfs_defer_pending *dfp = NULL; int error = 0; LIST_HEAD(dop_pending); @@ -506,17 +509,19 @@ xfs_defer_finish_noroll( * of time that any one intent item can stick around in memory, * pinning the log tail. */ - xfs_defer_create_intents(*tp); + bool has_intents = xfs_defer_create_intents(*tp); list_splice_init(&(*tp)->t_dfops, &dop_pending); - error = xfs_defer_trans_roll(tp); - if (error) - goto out_shutdown; + if (has_intents || dfp) { + error = xfs_defer_trans_roll(tp); + if (error) + goto out_shutdown; - /* Possibly relog intent items to keep the log moving. */ - error = xfs_defer_relog(tp, &dop_pending); - if (error) - goto out_shutdown; + /* Possibly relog intent items to keep the log moving. */ + error = xfs_defer_relog(tp, &dop_pending); + if (error) + goto out_shutdown; + } dfp = list_first_entry(&dop_pending, struct xfs_defer_pending, dfp_list);