Re: [PATCH v7 13/19] xfs: Add delay ready attr remove routines

Allison Collins
2020-02-27T04:19:04Z

On 2/26/20 3:34 PM, Dave Chinner wrote:
> On Tue, Feb 25, 2020 at 04:57:46PM -0800, Allison Collins wrote:
>> On 2/25/20 1:57 AM, Dave Chinner wrote:
>>> On Sat, Feb 22, 2020 at 07:06:05PM -0700, Allison Collins wrote:
>>>> +out:
>>>> + return error;
>>>> +}
>>>
>>> Brian commented on the structure of this loop better than I could.
>>>
>>>> +
>>>> +/*
>>>> + * Remove the attribute specified in @args.
>>>> + *
>>>> + * This function may return -EAGAIN to signal that the transaction needs to be
>>>> + * rolled. Callers should continue calling this function until they receive a
>>>> + * return value other than -EAGAIN.
>>>> + */
>>>> +int
>>>> +xfs_attr_remove_iter(
>>>> struct xfs_da_args *args)
>>>> {
>>>> struct xfs_inode *dp = args->dp;
>>>> int error;
>>>> + /* State machine switch */
>>>> + switch (args->dac.dela_state) {
>>>> + case XFS_DAS_RM_SHRINK:
>>>> + case XFS_DAS_RMTVAL_REMOVE:
>>>> + goto node;
>>>> + default:
>>>> + break;
>>>> + }
>>>
>>> Why separate out the state machine? Doesn't this shortcut the
>>> xfs_inode_hasattr() check? Shouldn't that come first?
>> Well, the idea is that when we first start the routine, we come in with
>> neither state set, and we fall through to the break. So we execute the
>> check the first time through.
>>
>> Though now that you point it out, I should probably go back and put the
>> explicit numbering back in the enum (starting with 1) or they will default
>> to zero, which would be incorrect. I had pulled it out in one of the last
>> reviews thinking it would be ok, but it should go back in.
>>
>>>
>>> As it is:
>>>
>>> case XFS_DAS_RM_SHRINK:
>>> case XFS_DAS_RMTVAL_REMOVE:
>>> return xfs_attr_node_removename(args);
>>> default:
>>> break;
>>>
>>> would be nicer, and if this is the only way we can get to
>>> xfs_attr_node_removename(c, getting rid of it from the code
>>> below could be done, too.
>> Well, the remove path is a lot simpler than the set path, so that trick does
>> work here :-)
>>
>> The idea though was to establish "jump points" with the "XFS_DAS_*" states.
>> Based on the state, we jump back to where we were. We could break this
>> pattern for the remove path, but I dont think we'd want to do the same for
>> the others. The set routine is a really big function that would end up
>> being inside a really big switch!
>
> Right, which is why I think it should be factored into function
> calls first, then the switch statement simply becomes a small set of
> function calls.
>
> We use that pattern quite a bit in the da_btree code to call
> the correct dir/attr function based on the type of block we are
> manipulating (i.e. based on da_state context). e.g. xfs_da3_split(),
> xfs_da3_join(), etc.
I see, sure will do. The patches were ordered much that way in the last
version, so it wouldnt be hard to undo.

>
>>>> struct xfs_da_geometry *geo; /* da block geometry */
>>>> struct xfs_name name; /* name, length and argument flags*/
>>>> uint8_t filetype; /* filetype of inode for directories */
>>>> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
>>>> index 1887605..9a649d1 100644
>>>> --- a/fs/xfs/scrub/common.c
>>>> +++ b/fs/xfs/scrub/common.c
>>>> @@ -24,6 +24,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/scrub.h"
>>>
>>> Hmmm - why are these new includes necessary? You didn't add anything
>>> new to these files or common header files to make the includes
>>> needed....
>>
>> Because the delayed attr context uses things from those headers. And we put
>> the context in xfs_da_args. Now everything that uses xfs_da_args needs
>> those includes. But maybe if we do what you suggest above, we wont need to.
>> :-)
>
> put:
>
> struct xfs_da_state;
>
> and whatever other forward declarations are require for the pointer
> types used in the delayed attr context at the top of xfs_attr.h.
>
> These are just pointers in the structure, so we don't need the full
> structure definitions if the pointers aren't actually dereferenced
> by the code that includes the header file.
Alrighty, will fix.

Thanks for the reviews!
Allison

>
> Cheers,
>
> Dave.
>