Re: [PATCH v4 16/17] xfs: Add delay ready attr remove routines

Allison Collins
2019-11-14T17:59:04Z

On 11/14/19 5:48 AM, Brian Foster wrote:
> On Wed, Nov 13, 2019 at 04:39:18PM -0700, Allison Collins wrote:
>>
>>
>> On 11/13/19 4:54 AM, Brian Foster wrote:

<snip>

>> Yes, as it is, it doesn't really have any way of detecting a corrupt state.
>> Really though, if we want to do that what we need is another layer of
>> modeling where we enforce the sequence of the states. So for example, some
>> sort of mapping or tree that defines: XFS_DC_LEAF_TO_NODE and
>> XFS_DC_FOUND_LBLK as being children (or mappings) of XFS_DC_SF_TO_LEAF.
>>
>> In other words, if the state is XFS_DC_SF_TO_LEAF, you can change it to
>> XFS_DC_FOUND_LBLK or XFS_DC_LEAF_TO_NODE, but you can't go to say
>> XFS_DC_RM_SHRINK, that would generate an error. And then we have some sort
>> of set_state() routine to enforce these rules.
>>
>> That's a lot of extra overhead though, and could be more of a burden than a
>> help. But it's something we could explore if people think it's worth the
>> pursuit.
>>
>
> Yeah, I don't think it's necessary to go so far as to create a complex
> set of rules and whatnot purely for the purpose of validating state
> management. I'm just handwaving about potentially structuring the code
> such that the state management is clear and easy to follow. :)
>
> Brian

Yeah, that's kinda how I felt too. Maybe later we could come back and
reconsider it depending on how the refactoring goes. I've done a little
research online around about state machine designs, and usually what I
see is some sort of mapping of state codes to function pointers. So if
it's possible to modularize everything into helpers, then an extra layer
of abstraction starts to make a lot more sense. But what we wouldn't
want is so much refactoring that we have a bunch of little helpers so
small and fragmented that the code flow is a headache to follow. So I
figure maybe for now we could just stick to refactoring snippets under
the switch approach, and if it starts to look like something that could
really benefit from a fully formed state machine model, we can consider
it then :-)

Allison

>
>> Allison
>>>
>>>> Thoughts?
>>>>
>>>>>
>>>>>> + }
>>>>>> error = xfs_attr_node_hasname(args, &state);
>>>>>> if (error != -EEXIST)
>>>>>> goto out;
>>>>>> + else
>>>>>> + error = 0;
>>>>>> /*
>>>>>> * If there is an out-of-line value, de-allocate the blocks.
>>>>>> @@ -1237,6 +1303,14 @@ xfs_attr_node_removename(
>>>>>> blk = &state->path.blk[ state->path.active-1 ];
>>>>>> ASSERT(blk->bp != NULL);
>>>>>> ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
>>>>>> +
>>>>>> + /*
>>>>>> + * Store blk and state in the context incase we need to cycle out the
>>>>>> + * transaction
>>>>>> + */
>>>>>> + args->dc.blk = blk;
>>>>>> + args->dc.da_state = state;
>>>>>> +
>>>>>> if (args->rmtblkno > 0) {
>>>>>> /*
>>>>>> * Fill in disk block numbers in the state structure
>>>>>> @@ -1255,13 +1329,30 @@ xfs_attr_node_removename(
>>>>>> if (error)
>>>>>> goto out;
>>>>>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>>>>>> + args->dc.dc_state = XFS_DC_RM_INVALIDATE;
>>>>>> + return -EAGAIN;
>>>>>> +rm_invalidate:
>>>>>> + error = xfs_attr_rmtval_invalidate(args);
>>>>>> if (error)
>>>>>> goto out;
>>>>>> +rm_node_blks:
>>>>>
>>>>> While I think the design is the right idea, jumping down into a function
>>>>> like this is pretty hairy. I think we should try to further break this
>>>>> function down into smaller elements one way or another that model the
>>>>> steps defined by the state structure. There's probably multiple ways to
>>>>> do that. For example, the remote attr bits could be broken down into
>>>>> a subfunction that processes the couple of states associated with remote
>>>>> blocks. That said, ISTM it might be wiser to try and keep the state
>>>>> processing in one place if possible. That would imply to break the
>>>>> remote processing loop down into a couple functions. All in all, this
>>>>> function might end up looking something like:
>>>>>
>>>>> xfs_attr_node_removename()
>>>>> {
>>>>> /* switch statement and comment to document each state */
>>>>>
>>>>> error = xfs_attr_node_hasname(args, &state);
>>>>> ...
>>>>>
>>>>> if (remote) {
>>>>> error = do_setflag();
>>>>> if (error)
>>>>> return error;
>>>>>
>>>>> /* roll */
>>>>> state = INVALIDATE;
>>>>> return -EAGAIN;
>>>>> }
>>>>>
>>>>> rmt_invalidate:
>>>>> state = INVALIDATE;
>>>>> if (remote)
>>>>> do_invalidate();
>>>>> /* fallthru */
>>>>>
>>>>> rmt_rm_blks:
>>>>> state = RM_NODE_BLKS;
>>>>> if (remote) {
>>>>> /* loops and returns -EAGAIN until we fallthru */
>>>>> error = rmt_remove_step();
>>>>> if (error)
>>>>> return error;
>>>>>
>>>>> xfs_attr_refillstate();
>>>>> }
>>>>>
>>>>> /* maybe worth a new state here? */
>>>>> removename:
>>>>> state = REMOVENAME;
>>>>> xfs_attr3_leaf_remove();
>>>>> ...
>>>>> if (...) {
>>>>> state = SHRINK;
>>>>> return -EAGAIN;
>>>>> }
>>>>>
>>>>> shrink:
>>>>> state = SHRINK;
>>>>> error = do_shrink();
>>>>>
>>>>> return 0;
>>>>> }
>>>> Ok, I had to go over this a few times, but I think I understand what you're
>>>> describing. Will update in the next version
>>>>>
>>>>> I'm not totally sold on the idea of rolling the state forward explicitly
>>>>> like this, but it seems like it could be a bit more maintainable.
>>>> I think it is. Having a dedicated struct just for this purpose alleviates a
>>>> lot of struggle with trying to grab onto things like the fork or the
>>>> incomplete flags to represent what we're trying to do here. Doing so also
>>>> overloads their original intent in that if these structures ever change in
>>>> the future, it may break something that the state machine depends on. In
>>>> this solution, they remain disjoint concepts dedicated to their purpose.
>>>> And anyway, I couldn't completely escape the state machine in the previous
>>>> set. I still had to add the extra flag space which functioned more or less
>>>> like "i was here" tick marks. If we have to have it, we may as well
>>>> leverage what it can do. For example I can drop patch 11 from this set
>>>> because I don't need the extra isset helpers to see if it's already been
>>>> done.
>>>>
>>>
>>> Right.. I agree that the "bookmark" like approach in the current scheme
>>> makes the state implementation (and not necessarily the operational
>>> implementation) a little hard to follow and review. Note again that what
>>> I wrote up here was just a quick example for that higher level feedback
>>> of somehow or another trying to isolate state updates from state
>>> implementation, so I'm not necessarily tied to that specific approach if
>>> there are other ways to similarly simplify things.
>>>
>>> I do think fixing up the code to avoid jumping into loops and whatnot is
>>> more important. It could also be that just continuing to break things
>>> down into as small functions as possible (i.e. with a goal of 1 function
>>> per state) kind of forces a natural separation.
>>>
>>> Brian
>>>
>>>> All in
>>>>> all this is still fairly ugly, but this is mostly a mechanical attempt
>>>>> to keep state management isolated and we can polish it up from there.
>>>>> Thoughts?
>>>>
>>>> Yes, at this point, I do kind of feel like it's the least of the ugly
>>>> prototypes. So I'm just kind of proceeding, with caution. :-)
>>>>
>>>> Thanks for the in depths reviews!! I know its a lot! Much appreciated!!
>>>>
>>>> Allison
>>>>
>>>>>
>>>>> Brian
>>>>>
>>>>>> + /*
>>>>>> + * Unmap value blocks for this attr. This is similar to
>>>>>> + * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
>>>>>> + * for new transactions
>>>>>> + */
>>>>>> + while (!done && !error) {
>>>>>> + error = xfs_bunmapi(args->trans, args->dp,
>>>>>> + args->rmtblkno, args->rmtblkcnt,
>>>>>> + XFS_BMAPI_ATTRFORK, 1, &done);
>>>>>> + if (error)
>>>>>> + return error;
>>>>>> - error = xfs_attr_rmtval_remove(args);
>>>>>> - if (error)
>>>>>> - goto out;
>>>>>> + if (!done) {
>>>>>> + args->dc.dc_state = XFS_DC_RM_NODE_BLKS;
>>>>>> + return -EAGAIN;
>>>>>> + }
>>>>>> + }
>>>>>> /*
>>>>>> * Refill the state structure with buffers, the prior calls
>>>>>> @@ -1287,17 +1378,12 @@ xfs_attr_node_removename(
>>>>>> error = xfs_da3_join(state);
>>>>>> if (error)
>>>>>> goto out;
>>>>>> - error = xfs_defer_finish(&args->trans);
>>>>>> - if (error)
>>>>>> - goto out;
>>>>>> - /*
>>>>>> - * Commit the Btree join operation and start a new trans.
>>>>>> - */
>>>>>> - error = xfs_trans_roll_inode(&args->trans, dp);
>>>>>> - if (error)
>>>>>> - goto out;
>>>>>> +
>>>>>> + args->dc.dc_state = XFS_DC_RM_SHRINK;
>>>>>> + return -EAGAIN;
>>>>>> }
>>>>>> +rm_shrink:
>>>>>> /*
>>>>>> * If the result is small enough, push it all into the inode.
>>>>>> */
>>>>>> @@ -1319,9 +1405,6 @@ xfs_attr_node_removename(
>>>>>> /* bp is gone due to xfs_da_shrink_inode */
>>>>>> if (error)
>>>>>> goto out;
>>>>>> - error = xfs_defer_finish(&args->trans);
>>>>>> - if (error)
>>>>>> - goto out;
>>>>>> } else
>>>>>> xfs_trans_brelse(args->trans, bp);
>>>>>> }
>>>>>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
>>>>>> index 3b5dad4..fb8bf5b 100644
>>>>>> --- a/fs/xfs/libxfs/xfs_attr.h
>>>>>> +++ b/fs/xfs/libxfs/xfs_attr.h
>>>>>> @@ -152,6 +152,7 @@ int xfs_attr_set_args(struct xfs_da_args *args);
>>>>>> int xfs_attr_remove(struct xfs_inode *dp, struct xfs_name *name, int flags);
>>>>>> int xfs_has_attr(struct xfs_da_args *args);
>>>>>> int xfs_attr_remove_args(struct xfs_da_args *args);
>>>>>> +int xfs_attr_remove_later(struct xfs_da_args *args);
>>>>>> int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
>>>>>> int flags, struct attrlist_cursor_kern *cursor);
>>>>>> bool xfs_attr_namecheck(const void *name, size_t length);
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>>
>>>>
>>>
>>
>