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

Allison Henderson
2019-04-16T02:30:58Z

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
>>