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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> > > > > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > > > ---
> > > > >    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...

--D

> Allison
> 
> > 
> > Dave.
> > 
> 
> 



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux