diff mbox series

[1/9] xfs: don't set XFS_TRANS_HAS_INTENT_DONE when there's no ATTRD log item

Message ID 170162990183.3037772.16569536668272771929.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded
Headers show
Series xfs: continue removing defer item boilerplate | expand

Commit Message

Darrick J. Wong Dec. 3, 2023, 7:02 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

XFS_TRANS_HAS_INTENT_DONE is a flag to the CIL that we've added a log
intent done item to the transaction.  This enables an optimization
wherein we avoid writing out log intent and log intent done items if
they would have ended up in the same checkpoint.  This reduces writes to
the ondisk log and speeds up recovery as a result.

However, callers can use the defer ops machinery to modify xattrs
without using the log items.  In this situation, there won't be an
intent done item, so we do not need to set the flag.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_attr_item.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Dec. 4, 2023, 5:08 a.m. UTC | #1 
On Sun, Dec 03, 2023 at 11:02:57AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> XFS_TRANS_HAS_INTENT_DONE is a flag to the CIL that we've added a log
> intent done item to the transaction.  This enables an optimization
> wherein we avoid writing out log intent and log intent done items if
> they would have ended up in the same checkpoint.  This reduces writes to
> the ondisk log and speeds up recovery as a result.
> 
> However, callers can use the defer ops machinery to modify xattrs
> without using the log items.  In this situation, there won't be an
> intent done item, so we do not need to set the flag.

Understanding the logged attrs code is till on my TODO list, but
the patch looks obviously correct in that we shouldn't set
XFS_TRANS_HAS_INTENT_DONE if there is no done items.  I'm still
confused how it can log an intent item without a done item,
though.

Cautiously and superficially:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Dec. 4, 2023, 6:43 p.m. UTC | #2 
On Mon, Dec 04, 2023 at 06:08:03AM +0100, Christoph Hellwig wrote:
> On Sun, Dec 03, 2023 at 11:02:57AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > XFS_TRANS_HAS_INTENT_DONE is a flag to the CIL that we've added a log
> > intent done item to the transaction.  This enables an optimization
> > wherein we avoid writing out log intent and log intent done items if
> > they would have ended up in the same checkpoint.  This reduces writes to
> > the ondisk log and speeds up recovery as a result.
> > 
> > However, callers can use the defer ops machinery to modify xattrs
> > without using the log items.  In this situation, there won't be an
> > intent done item, so we do not need to set the flag.
> 
> Understanding the logged attrs code is till on my TODO list, but
> the patch looks obviously correct in that we shouldn't set
> XFS_TRANS_HAS_INTENT_DONE if there is no done items.  I'm still
> confused how it can log an intent item without a done item,
> though.

Dave and Allison and I at some point realized that the defer ops
machinery works even if ->create_intent and ->create_done return NULL.
You'd lose the ability to restart the operation after a crash, but if
the upper layers can tolerate a half-finished operation
(e.g.  ATTR_INCOMPLETE) then that should be ok.

Obviously you wouldn't touch any such *existing* code except as part of
adapting it to be capable of using log items, and that's exactly what
Allison did.  She refactor the old xattr code to track the state of the
operation explicitly, then moved all that into the ->finish_item
implementation.  Now, if the setattr operation does not set the LOGGED
flag (the default), the behavior should be exactly the same as before.
If they do set LOGGED (either because the debug knob is set; or because
the caller is parent pointers) then ->create_{intent,done} actually
create log intent and done items.

It should never create an intent item and not the done item or the other
way 'round, obviously.  Either both functions return NULL, or they both
return non-NULL.

--D

> Cautiously and superficially:

Thanks! :)

> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
Christoph Hellwig Dec. 4, 2023, 7:44 p.m. UTC | #3 
On Mon, Dec 04, 2023 at 10:43:48AM -0800, Darrick J. Wong wrote:
> Dave and Allison and I at some point realized that the defer ops
> machinery works even if ->create_intent and ->create_done return NULL.
> You'd lose the ability to restart the operation after a crash, but if
> the upper layers can tolerate a half-finished operation
> (e.g.  ATTR_INCOMPLETE) then that should be ok.
> 
> Obviously you wouldn't touch any such *existing* code except as part of
> adapting it to be capable of using log items, and that's exactly what
> Allison did.  She refactor the old xattr code to track the state of the
> operation explicitly, then moved all that into the ->finish_item
> implementation.  Now, if the setattr operation does not set the LOGGED
> flag (the default), the behavior should be exactly the same as before.
> If they do set LOGGED (either because the debug knob is set; or because
> the caller is parent pointers) then ->create_{intent,done} actually
> create log intent and done items.
> 
> It should never create an intent item and not the done item or the other
> way 'round, obviously.  Either both functions return NULL, or they both
> return non-NULL.

It would be really good to document this, the name LARP and why it is
considered a debug feature somewhere in the tree.  No need to hold
up this series for that of course.
Darrick J. Wong Dec. 4, 2023, 8:34 p.m. UTC | #4 
On Mon, Dec 04, 2023 at 08:44:45PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 04, 2023 at 10:43:48AM -0800, Darrick J. Wong wrote:
> > Dave and Allison and I at some point realized that the defer ops
> > machinery works even if ->create_intent and ->create_done return NULL.
> > You'd lose the ability to restart the operation after a crash, but if
> > the upper layers can tolerate a half-finished operation
> > (e.g.  ATTR_INCOMPLETE) then that should be ok.
> > 
> > Obviously you wouldn't touch any such *existing* code except as part of
> > adapting it to be capable of using log items, and that's exactly what
> > Allison did.  She refactor the old xattr code to track the state of the
> > operation explicitly, then moved all that into the ->finish_item
> > implementation.  Now, if the setattr operation does not set the LOGGED
> > flag (the default), the behavior should be exactly the same as before.
> > If they do set LOGGED (either because the debug knob is set; or because
> > the caller is parent pointers) then ->create_{intent,done} actually
> > create log intent and done items.
> > 
> > It should never create an intent item and not the done item or the other
> > way 'round, obviously.  Either both functions return NULL, or they both
> > return non-NULL.
> 
> It would be really good to document this, the name LARP and why it is
> considered a debug feature somewhere in the tree.  No need to hold
> up this series for that of course.

Yeah, that'll become a third cleanup series to add a comment and elide
tthe create_done thing. :)

--D
diff mbox series

Patch

diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 5f7d8e8d87dc..1b7f1313f51e 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -347,13 +347,15 @@  xfs_xattri_finish_update(
 	 * 1.) releases the ATTRI and frees the ATTRD
 	 * 2.) shuts down the filesystem
 	 */
-	args->trans->t_flags |= XFS_TRANS_DIRTY | XFS_TRANS_HAS_INTENT_DONE;
+	args->trans->t_flags |= XFS_TRANS_DIRTY;
 
 	/*
 	 * attr intent/done items are null when logged attributes are disabled
 	 */
-	if (attrdp)
+	if (attrdp) {
+		args->trans->t_flags |= XFS_TRANS_HAS_INTENT_DONE;
 		set_bit(XFS_LI_DIRTY, &attrdp->attrd_item.li_flags);
+	}
 
 	return error;
 }