Re: [PATCH v7 02/19] xfs: Embed struct xfs_name in xfs_da_args

Allison Collins
2020-02-25T06:07:43Z

On 2/24/20 9:27 PM, Darrick J. Wong wrote:
> On Mon, Feb 24, 2020 at 09:19:58PM -0700, Allison Collins wrote:
>>
>>
>> On 2/24/20 9:06 PM, Dave Chinner wrote:
>>> On Mon, Feb 24, 2020 at 07:00:35PM -0700, Allison Collins wrote:
>>>>
>>>>
>>>> On 2/24/20 5:57 PM, Dave Chinner wrote:
>>>>> On Sat, Feb 22, 2020 at 07:05:54PM -0700, Allison Collins wrote:
>>>>>> This patch embeds an xfs_name in xfs_da_args, replacing the name, namelen, and flags
>>>>>> members. This helps to clean up the xfs_da_args structure and make it more uniform
>>>>>> with the new xfs_name parameter being passed around.
>>>>>
>>>>> Commit message should wrap at 68-72 columns.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>>>>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>>>>>> ---
>>>>>> fs/xfs/libxfs/xfs_attr.c | 37 +++++++-------
>>>>>> fs/xfs/libxfs/xfs_attr_leaf.c | 104 +++++++++++++++++++++-------------------
>>>>>> fs/xfs/libxfs/xfs_attr_remote.c | 2 +-
>>>>>> fs/xfs/libxfs/xfs_da_btree.c | 6 ++-
>>>>>> fs/xfs/libxfs/xfs_da_btree.h | 4 +-
>>>>>> fs/xfs/libxfs/xfs_dir2.c | 18 +++----
>>>>>> fs/xfs/libxfs/xfs_dir2_block.c | 6 +--
>>>>>> fs/xfs/libxfs/xfs_dir2_leaf.c | 6 +--
>>>>>> fs/xfs/libxfs/xfs_dir2_node.c | 8 ++--
>>>>>> fs/xfs/libxfs/xfs_dir2_sf.c | 30 ++++++------
>>>>>> fs/xfs/scrub/attr.c | 12 ++---
>>>>>> fs/xfs/xfs_trace.h | 20 ++++----
>>>>>> 12 files changed, 130 insertions(+), 123 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>>>>> index 6717f47..9acdb23 100644
>>>>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>>>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>>>>> @@ -72,13 +72,12 @@ xfs_attr_args_init(
>>>>>> args->geo = dp->i_mount->m_attr_geo;
>>>>>> args->whichfork = XFS_ATTR_FORK;
>>>>>> args->dp = dp;
>>>>>> - args->flags = flags;
>>>>>> - args->name = name->name;
>>>>>> - args->namelen = name->len;
>>>>>> - if (args->namelen >= MAXNAMELEN)
>>>>>> + memcpy(&args->name, name, sizeof(struct xfs_name));
>>>>>> + args->name.type = flags;
>>>>>
>>>>> This doesn't play well with Christoph's cleanup series which fixes
>>>>> up all the confusion with internal versus API flags. I guess the
>>>>> namespace is part of the attribute name, but I think this would be a
>>>>> much clearer conversion when placed on top of the way Christoph
>>>>> cleaned all this up...
>>>>>
>>>>> Have you looked at rebasing this on top of that cleanup series?
>>>>>
>>>>> Cheers,
>>>>>
>>>> Yes, there is some conflict between the sets here and there, but I think
>>>> folks wanted to keep them separate for now.
>>>
>>> That makes it really hard to form a clear view of what the code
>>> looks like after both patchsets have been applied. :(
>>>
>>>> Are you referring to
>>>> "[780d29057781] xfs: fix misuse of the XFS_ATTR_INCOMPLETE flag"? I'm
>>>> pretty sure this set is already seated on top of that one. This one is
>>>> based on the latest for-next.
>>>
>>> No, I'm talking about the series that ends up undoing that commit
>>> (i.e. the DA_OP_INCOMPLETE flag goes away again) and turns
>>> args->flags into args->attr_filter as the namespace filter for
>>> lookups. THis also turn adds XFS_ATTR_INCOMPLETE into a lookup
>>> filter.
>>>
>>> With this separation of ops vs lookup filters, moving the lookup
>>> filter into the xfs_name makes a bit more sense (i.e. the namespace
>>> filter is passed with the attribute name), but as a standalone
>>> movement it creates a bit of an impedence mismatch between the xname
>>> and the use of these flags.
>>>
>>> I think the end result will be fine, but it's making it hard for me
>>> to reconcile the changes in the two patchsets...
>>>
>>> Cheers,
>>
>> I'd be happy to go through the sets and find the intersections. Or make a
>> big super set if you like. I got the impression though that Christoph didnt
>> particularly like the delayed attr series or the idea of blending them. But
>> I do think it would be a good idea to take into consideration what the
>> combination of them is going to look like.
>
> At this point you might as well wait for me to actually put hch's attr
> interface refactoring series into for-next (unless this series is
> already based off of that??) though Christoph might be a bit time
> constrained this week...

It's based on for-next at this point. Both these sets are pretty big,
so it's a lot to chase if he's not interested in slowing down to work
with the combination of them. If folks would like to see the
combination set before moving forward, I'd be happy to try and put that
together. Otherwise, it can wait too. Let me know. Thanks!

Allison

>
> --D
>
>> Allison
>>
>>>
>>> Dave.
>>>
>>
>>