[PATCHSET 0/2] xfs: elide defer work ->create_done if no intent

[PATCHSET 0/2] xfs: elide defer work ->create_done if no intent

[Darrick J. Wong]

Hi all,

Christoph pointed out that the defer ops machinery doesn't need to call
->create_done if the deferred work item didn't generate a log intent
item in the first place.  Let's clean that up and save an indirect call
in the non-logged xattr update call path.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=defer-elide-create-done-6.7
---
 fs/xfs/libxfs/xfs_defer.c |    4 ++++
 fs/xfs/xfs_attr_item.c    |    3 ---
 fs/xfs/xfs_sysfs.c        |    7 +++++++
 3 files changed, 11 insertions(+), 3 deletions(-)

[PATCH 1/2] xfs: document what LARP means

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Christoph requested a blurb somewhere explaining exactly what LARP
means.  I don't know of a good place other than the source code (debug
knobs aren't covered in Documentation/), so here it is.

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


diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index a3c6b1548723..59869a1ee49f 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -229,6 +229,13 @@ pwork_threads_show(
 }
 XFS_SYSFS_ATTR_RW(pwork_threads);
 
+/*
+ * The "LARP" (Logged extended Attribute Recovery Persistence) debugging knob
+ * sets the XFS_DA_OP_LOGGED flag on all xfs_attr_set operations performed on
+ * V5 filesystems.  As a result, the intermediate progress of all setxattr and
+ * removexattr operations are tracked via the log and can be restarted during
+ * recovery.
+ */
 static ssize_t
 larp_store(
 	struct kobject	*kobject,

Re: [PATCH 1/2] xfs: document what LARP means

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 09:36:07PM -0800, Darrick J. Wong wrote:
> +/*
> + * The "LARP" (Logged extended Attribute Recovery Persistence) debugging knob
> + * sets the XFS_DA_OP_LOGGED flag on all xfs_attr_set operations performed on
> + * V5 filesystems.  As a result, the intermediate progress of all setxattr and
> + * removexattr operations are tracked via the log and can be restarted during
> + * recovery.
> + */

Can you also add a sentence on why we even have this code and why you'd
want to set the flag?

Re: [PATCH 1/2] xfs: document what LARP means

[Darrick J. Wong]

On Tue, Dec 05, 2023 at 06:38:42AM +0100, Christoph Hellwig wrote:
> On Mon, Dec 04, 2023 at 09:36:07PM -0800, Darrick J. Wong wrote:
> > +/*
> > + * The "LARP" (Logged extended Attribute Recovery Persistence) debugging knob
> > + * sets the XFS_DA_OP_LOGGED flag on all xfs_attr_set operations performed on
> > + * V5 filesystems.  As a result, the intermediate progress of all setxattr and
> > + * removexattr operations are tracked via the log and can be restarted during
> > + * recovery.
> > + */
> 
> Can you also add a sentence on why we even have this code and why you'd
> want to set the flag?

How about these last couple of sentences?

/*
 * The "LARP" (Logged extended Attribute Recovery Persistence) debugging knob
 * sets the XFS_DA_OP_LOGGED flag on all xfs_attr_set operations performed on
 * V5 filesystems.  As a result, the intermediate progress of all setxattr and
 * removexattr operations are tracked via the log and can be restarted during
 * recovery.  This is useful for testing xattr recovery prior to merging of the
 * parent pointer feature which requires it to maintain consistency, and may be
 * enabled for userspace xattrs in the future.
 */

--D

Re: [PATCH 1/2] xfs: document what LARP means

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 09:50:28PM -0800, Darrick J. Wong wrote:
> How about these last couple of sentences?
> 
> /*
>  * The "LARP" (Logged extended Attribute Recovery Persistence) debugging knob
>  * sets the XFS_DA_OP_LOGGED flag on all xfs_attr_set operations performed on
>  * V5 filesystems.  As a result, the intermediate progress of all setxattr and
>  * removexattr operations are tracked via the log and can be restarted during
>  * recovery.  This is useful for testing xattr recovery prior to merging of the
>  * parent pointer feature which requires it to maintain consistency, and may be
>  * enabled for userspace xattrs in the future.
>  */

Oooh.  So all the logged attrs work is preparation for parent pointers?
That makes a whole lot of sense, but I've missed it so far.  Yes, the
above comment is great.

With that:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] xfs: document what LARP means

[Darrick J. Wong]

On Tue, Dec 05, 2023 at 06:56:42AM +0100, Christoph Hellwig wrote:
> On Mon, Dec 04, 2023 at 09:50:28PM -0800, Darrick J. Wong wrote:
> > How about these last couple of sentences?
> > 
> > /*
> >  * The "LARP" (Logged extended Attribute Recovery Persistence) debugging knob
> >  * sets the XFS_DA_OP_LOGGED flag on all xfs_attr_set operations performed on
> >  * V5 filesystems.  As a result, the intermediate progress of all setxattr and
> >  * removexattr operations are tracked via the log and can be restarted during
> >  * recovery.  This is useful for testing xattr recovery prior to merging of the
> >  * parent pointer feature which requires it to maintain consistency, and may be
> >  * enabled for userspace xattrs in the future.
> >  */
> 
> Oooh.  So all the logged attrs work is preparation for parent pointers?
> That makes a whole lot of sense, but I've missed it so far.  Yes, the
> above comment is great.

Yep!  And that's coming in the year end megapatchbomb. :)

--D

> With that:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

[PATCH 2/2] xfs: elide ->create_done calls for unlogged deferred work

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Extended attribute updates use the deferred work machinery to manage
state across a chain of smaller transactions.  All previous deferred
work users have employed log intent items and log done items to manage
restarting of interrupted operations, which means that ->create_intent
sets dfp_intent to a log intent item and ->create_done uses that item to
create a log intent done item.

However, xattrs have used the INCOMPLETE flag to deal with the lack of
recovery support for an interrupted transaction chain.  Log items are
optional if the xattr update caller didn't set XFS_DA_OP_LOGGED to
require a restartable sequence.

In other words, ->create_intent can return NULL to say that there's no
log intent item.  If that's the case, no log intent done item should be
created.  Clean up xfs_defer_create_done not to do this, so that the
->create_done functions don't have to check for non-null dfp_intent
themselves.

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


diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 54a6be06e6cd..06e890b44c52 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -201,6 +201,10 @@ xfs_defer_create_done(
 	const struct xfs_defer_op_type	*ops = defer_op_types[dfp->dfp_type];
 	struct xfs_log_item		*lip;
 
+	/* If there is no log intent item, there can be no log done item. */
+	if (!dfp->dfp_intent)
+		return;
+
 	/*
 	 * Mark the transaction dirty, even on error. This ensures the
 	 * transaction is aborted, which:
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 53d34f689173..e5bcb16b88f4 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -737,9 +737,6 @@ xfs_attr_create_done(
 	struct xfs_attri_log_item	*attrip;
 	struct xfs_attrd_log_item	*attrdp;
 
-	if (!intent)
-		return NULL;
-
 	attrip = ATTRI_ITEM(intent);
 
 	attrdp = kmem_cache_zalloc(xfs_attrd_cache, GFP_NOFS | __GFP_NOFAIL);

Re: [PATCH 2/2] xfs: elide ->create_done calls for unlogged deferred work

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHSET v2 0/2] xfs: elide defer work ->create_done if no intent

[Darrick J. Wong]

Hi all,

Christoph pointed out that the defer ops machinery doesn't need to call
->create_done if the deferred work item didn't generate a log intent
item in the first place.  Let's clean that up and save an indirect call
in the non-logged xattr update call path.

v2: pick up rvb tags

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=defer-elide-create-done-6.8
---
 fs/xfs/libxfs/xfs_defer.c |    4 ++++
 fs/xfs/xfs_attr_item.c    |    3 ---
 fs/xfs/xfs_sysfs.c        |    9 +++++++++
 3 files changed, 13 insertions(+), 3 deletions(-)

[PATCH 1/2] xfs: document what LARP means

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Christoph requested a blurb somewhere explaining exactly what LARP
means.  I don't know of a good place other than the source code (debug
knobs aren't covered in Documentation/), so here it is.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_sysfs.c |    9 +++++++++
 1 file changed, 9 insertions(+)


diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index a3c6b1548723..871f16a4a5d8 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -229,6 +229,15 @@ pwork_threads_show(
 }
 XFS_SYSFS_ATTR_RW(pwork_threads);
 
+/*
+ * The "LARP" (Logged extended Attribute Recovery Persistence) debugging knob
+ * sets the XFS_DA_OP_LOGGED flag on all xfs_attr_set operations performed on
+ * V5 filesystems.  As a result, the intermediate progress of all setxattr and
+ * removexattr operations are tracked via the log and can be restarted during
+ * recovery.  This is useful for testing xattr recovery prior to merging of the
+ * parent pointer feature which requires it to maintain consistency, and may be
+ * enabled for userspace xattrs in the future.
+ */
 static ssize_t
 larp_store(
 	struct kobject	*kobject,