Re: [PATCH v5 04/14] xfs: Add xfs_has_attr and subroutines

Allison Collins
2020-01-26T04:04:14Z

On 1/25/20 4:08 PM, Christoph Hellwig wrote:
> On Sat, Jan 25, 2020 at 09:27:39AM -0700, Allison Collins wrote:
>> I was thinking of adapting this patch to be part of a 3 patch series
>> including one of Chistophs. Something like this:
>>
>> [PATCH v6 11/16] xfs: Check for -ENOATTR or -EEXIST
>> [PATCH v6 03/16] xfs: Add xfs_has_attr and subroutines
>> [PATCH 02/29] xfs: merge xfs_attr_remove into xfs_attr_set
>>
>> What would people think of that? I figured this would be better than the
>> two of us bombarding the mailing list with giant conflicting sets? Also, was
>> I able to answer everyones questions on this patch?
>
> I'm still not sold at all on this series. It adds a lot of code and
> makes it much harder to understand. So I'd much rather go back and
> figuring out how we can do delayed attrs in a more streamlined way.

I had something like that quite a long time ago. Instead of the EAGAIN
ping pong, we had a boolean that just sort of turned the transactions
off in the delayed attrs path:

https://www.spinics.net/lists/linux-xfs/msg18114.html

The trouble with this is that it creates one large transaction rather
than breaking them up. So while the new method is a lot more complex,
it seemed to be the direction folks preferred.

> The has_attr and co changes are some of exactly that kind of logic
> that is just making things worse in the standalone patch set, so even
> if we must end up with it they absolutely belong into a series actually
> adding functionality, as they have no use on their own. >
Well, in an earlier discussion though we had come together with a plan
of implementing the new feature through a series of sub sets. First of
which being pure refactor with no functional change.

https://www.spinics.net/lists/linux-xfs/msg32035.html

I do try to keep the extended set up to date though. Mostly for my own
sanity and to make sure things are still on track for the long term
goals we are aiming for.

> Independent of that we'll need to clean up the flags mess, so I'd rather
> just go ahead with that for now.
>