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

[Allison Collins <allison.henderson@oracle.com>]

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
>>>>>>
>>>>>
>>>>
>>>
>>
>