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

Allison Collins
2020-02-25T04:22:04Z

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.

Allison

>
> Dave.
>