Re: [PATCH 7/9] xfs: Add attr context to log item

[Allison Henderson <allison.henderson@oracle.com>]

On 4/15/19 3:50 PM,  wrote:
> On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote:
>> This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer
>> and a new state type. We will use these in the next patch when
>> we modify xfs_set_attr_args to roll transactions by returning EAGAIN.
>> Because the subroutines of this function modify the contents of these
>> structures, we need to find a place to store them where they remain
>> instantiated across multiple calls to xfs_set_attr_args.
>>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++-
>>   fs/xfs/scrub/common.c    |  2 ++
>>   fs/xfs/xfs_acl.c         |  2 ++
>>   fs/xfs/xfs_attr_item.c   |  2 +-
>>   fs/xfs/xfs_ioctl.c       |  2 ++
>>   fs/xfs/xfs_ioctl32.c     |  2 ++
>>   fs/xfs/xfs_iops.c        |  1 +
>>   fs/xfs/xfs_xattr.c       |  1 +
>>   8 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
>> index 974c963..4ce3b0a 100644
>> --- a/fs/xfs/libxfs/xfs_attr.h
>> +++ b/fs/xfs/libxfs/xfs_attr.h
>> @@ -77,6 +77,13 @@ typedef struct attrlist_ent {	/* data from attr_list() */
>>   	char	a_name[1];	/* attr name (NULL terminated) */
>>   } attrlist_ent_t;
>>   
>> +/* Attr state machine types */
>> +enum xfs_attr_state {
>> +	XFS_ATTR_STATE1 = 1,
>> +	XFS_ATTR_STATE2 = 2,
>> +	XFS_ATTR_STATE3 = 3,
> 
> Um... to what states do these refer?

I actually struggled with what to call these other than state machine 
types.  They are sort of "you were here" bookmark for xfs_attr_set_args. 
  The idea is that when we return EAGAIN, and then get recalled with a 
new transaction, we jump back to where we were based on this marker.

> 
>> +};
>> +
>>   /*
>>    * List of attrs to commit later.
>>    */
>> @@ -88,7 +95,16 @@ struct xfs_attr_item {
>>   	void		  *xattri_name;	      /* attr name */
>>   	uint32_t	  xattri_name_len;    /* length of name */
>>   	uint32_t	  xattri_flags;       /* attr flags */
>> -	struct list_head  xattri_list;
>> +
>> +	/*
>> +	 * Delayed attr parameters that need to remain instantiated
>> +	 * across transaction rolls during the defer finish
>> +	 */
>> +	struct xfs_buf		*xattri_leaf_bp;  /* Leaf buf to release */
>> +	enum xfs_attr_state	xattri_state;	  /* state machine marker */
>> +	struct xfs_da_args	xattri_args;	  /* args context */
> 
> Assuming we're keeping xattri_args.trans up to date here?

Yes, that happens in xfs_attr_finish_item in the next patch.

> 
>> +
>> +	struct list_head	xattri_list;
> 
> What's this for?

xattri_list is introduced in patch 2, which I loosely modeled off other 
delayed items at the time.  It a list of intents that have been logged 
to this item.  Though it could use a comment :-)

It doesn't relate directly to the "re-roll with EAGAIN" mechanics being 
added in this patch if that's what you are asking.  It just needs to be 
the last member of the struct because it's followed by a byte array.

I hope that helps to explain some.  Let me know if you have any other 
questions.  Thanks!

Allison
> 
> --D
> 
>>   
>>   	/*
>>   	 * A byte array follows the header containing the file name and
>> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
>> index 0c54ff5..270c32e 100644
>> --- a/fs/xfs/scrub/common.c
>> +++ b/fs/xfs/scrub/common.c
>> @@ -30,6 +30,8 @@
>>   #include "xfs_rmap_btree.h"
>>   #include "xfs_log.h"
>>   #include "xfs_trans_priv.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>>   #include "xfs_attr.h"
>>   #include "xfs_reflink.h"
>>   #include "scrub/xfs_scrub.h"
>> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
>> index 142de8d..9b1b93e 100644
>> --- a/fs/xfs/xfs_acl.c
>> +++ b/fs/xfs/xfs_acl.c
>> @@ -10,6 +10,8 @@
>>   #include "xfs_mount.h"
>>   #include "xfs_inode.h"
>>   #include "xfs_acl.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>>   #include "xfs_attr.h"
>>   #include "xfs_trace.h"
>>   #include <linux/slab.h>
>> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
>> index 0ea19b4..36e6d1e 100644
>> --- a/fs/xfs/xfs_attr_item.c
>> +++ b/fs/xfs/xfs_attr_item.c
>> @@ -19,10 +19,10 @@
>>   #include "xfs_rmap.h"
>>   #include "xfs_inode.h"
>>   #include "xfs_icache.h"
>> -#include "xfs_attr.h"
>>   #include "xfs_shared.h"
>>   #include "xfs_da_format.h"
>>   #include "xfs_da_btree.h"
>> +#include "xfs_attr.h"
>>   
>>   static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip)
>>   {
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index ab341d6..c8728ca 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -16,6 +16,8 @@
>>   #include "xfs_rtalloc.h"
>>   #include "xfs_itable.h"
>>   #include "xfs_error.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>>   #include "xfs_attr.h"
>>   #include "xfs_bmap.h"
>>   #include "xfs_bmap_util.h"
>> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
>> index 5001dca..23f6990 100644
>> --- a/fs/xfs/xfs_ioctl32.c
>> +++ b/fs/xfs/xfs_ioctl32.c
>> @@ -21,6 +21,8 @@
>>   #include "xfs_fsops.h"
>>   #include "xfs_alloc.h"
>>   #include "xfs_rtalloc.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>>   #include "xfs_attr.h"
>>   #include "xfs_ioctl.h"
>>   #include "xfs_ioctl32.h"
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index e73c21a..561c467 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -17,6 +17,7 @@
>>   #include "xfs_acl.h"
>>   #include "xfs_quota.h"
>>   #include "xfs_error.h"
>> +#include "xfs_da_btree.h"
>>   #include "xfs_attr.h"
>>   #include "xfs_trans.h"
>>   #include "xfs_trace.h"
>> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
>> index 3013746..938e81d 100644
>> --- a/fs/xfs/xfs_xattr.c
>> +++ b/fs/xfs/xfs_xattr.c
>> @@ -11,6 +11,7 @@
>>   #include "xfs_mount.h"
>>   #include "xfs_da_format.h"
>>   #include "xfs_inode.h"
>> +#include "xfs_da_btree.h"
>>   #include "xfs_attr.h"
>>   #include "xfs_attr_leaf.h"
>>   #include "xfs_acl.h"
>> -- 
>> 2.7.4
>>