Re: [PATCH v4 11/17] xfs: Add xfs_attr3_leaf helper functions

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

On 11/8/19 2:17 PM, Darrick J. Wong wrote:
> On Wed, Nov 06, 2019 at 06:27:55PM -0700, Allison Collins wrote:
>> And new helper functions xfs_attr3_leaf_flag_is_set and
>> xfs_attr3_leaf_flagsflipped.  These routines check to see
>> if xfs_attr3_leaf_setflag or xfs_attr3_leaf_flipflags have
>> already been run.  We will need this later for delayed
>> attributes since routines may be recalled several times
>> when -EAGAIN is returned.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr_leaf.c | 94 +++++++++++++++++++++++++++++++++++++++++++
>>   fs/xfs/libxfs/xfs_attr_leaf.h |  2 +
>>   2 files changed, 96 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
>> index 42c037e..023c616 100644
>> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
>> @@ -2809,6 +2809,40 @@ xfs_attr3_leaf_clearflag(
>>   }
>>   
>>   /*
>> + * Check if the INCOMPLETE flag on an entry in a leaf block is set.  This
>> + * function can be used to check if xfs_attr3_leaf_setflag has already been
>> + * called.  The INCOMPLETE flag is used during attr rename operations to mark
>> + * entries that are being renamed. Since renames should be atomic, only one of
> 
> It's also used when creating an xattr with a value stored in a remote
> block so that we can commit the name entry to the log (with INCOMPLETE
> set), allocate/write the remote value with ordered buffers, and then
> commit a second transaction clearing the INCOMPLETE flag.
> 
> Now that I think about it ... this predicate is for non-rename setting
> of attrs with remote values, and the "flagsflipped" predicate is for
> rename operations, aren't they?\

Yes, but I'm realizing now, that we may not need these helpers if we 
continue to move forward with the state machine.  With the state 
machine, we can simply flip the flag, set the next state, and move on 
since that region of code will not execute again.  The state switch will 
jump us forward to where we were without having to check if we already 
did it.

> 
>> + * them should appear as a completed attribute.
>> + *
>> + * isset is set to true if the flag is set or false otherwise
>> + */
>> +int
>> +xfs_attr3_leaf_flag_is_set(
>> +	struct xfs_da_args		*args,
>> +	bool				*isset)
>> +{
>> +	struct xfs_attr_leafblock	*leaf;
>> +	struct xfs_attr_leaf_entry	*entry;
>> +	struct xfs_buf			*bp;
>> +	struct xfs_inode		*dp = args->dp;
>> +	int				error = 0;
>> +
>> +	error = xfs_attr3_leaf_read(args->trans, dp, args->blkno,
>> +				    XFS_DABUF_MAP_NOMAPPING, &bp);
>> +	if (error)
>> +		return error;
>> +
>> +	leaf = bp->b_addr;
>> +	entry = &xfs_attr3_leaf_entryp(leaf)[args->index];
>> +
>> +	*isset = ((entry->flags & XFS_ATTR_INCOMPLETE) != 0);
>> +	xfs_trans_brelse(args->trans, bp);
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>>    * Set the INCOMPLETE flag on an entry in a leaf block.
>>    */
>>   int
>> @@ -2972,3 +3006,63 @@ xfs_attr3_leaf_flipflags(
>>   
>>   	return error;
>>   }
>> +
>> +/*
>> + * On a leaf entry, check to see if the INCOMPLETE flag is cleared
>> + * in args->blkno/index and set in args->blkno2/index2.
> 
> Might be worth mentioning here that args->blkno is the old entry and
> args->blkno2 is the new entry.  This predicate will be used (by the
> deferred attr item recovery code) to decide if we have to finish that
> part of a rename operation, right?
No, it was used in the previous series xfs_attr_set_later where I was 
aiming to rely more on the state of the fork rather than the state 
machine.  But with the new state machine in this series, I may be able 
to just drop this patch.

> 
>>   Note that they could be
>> + * in different blocks, or in the same block.  This function can be used to
>> + * check if xfs_attr3_leaf_flipflags has already been called.  The INCOMPLETE
>> + * flag is used during attr rename operations to mark entries that are being
>> + * renamed. Since renames should be atomic, only one of them should appear as a
>> + * completed attribute.
>> + *
>> + * isflipped is set to true if flags are flipped or false otherwise
>> + */
>> +int
>> +xfs_attr3_leaf_flagsflipped(
> 
> I don't like "flagsflipped" because it's not clear to me what "flipped"
> means.
> 
> xfs_attr3_leaf_rename_is_incomplete() ?
> 
Sure, if I keep this patch in the set I will update it.  Things are 
still pretty wiggly, so i may still find a use for it, but hopefully I 
can let it go.

>> +	struct xfs_da_args		*args,
>> +	bool				*isflipped)
>> +{
>> +	struct xfs_attr_leafblock	*leaf1;
>> +	struct xfs_attr_leafblock	*leaf2;
>> +	struct xfs_attr_leaf_entry	*entry1;
>> +	struct xfs_attr_leaf_entry	*entry2;
>> +	struct xfs_buf			*bp1;
>> +	struct xfs_buf			*bp2;
>> +	struct xfs_inode		*dp = args->dp;
>> +	int				error = 0;
>> +
>> +	/*
>> +	 * Read the block containing the "old" attr
>> +	 */
>> +	error = xfs_attr3_leaf_read(args->trans, dp, args->blkno,
>> +				    XFS_DABUF_MAP_NOMAPPING, &bp1);
>> +	if (error)
>> +		return error;
>> +
>> +	/*
>> +	 * Read the block containing the "new" attr, if it is different
>> +	 */
>> +	if (args->blkno2 != args->blkno) {
>> +		error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno2,
>> +					   -1, &bp2);
>> +		if (error)
> 
> bp1 leaks here, I think.
ok, will make a note.

> 
>> +			return error;
>> +	} else {
>> +		bp2 = bp1;
>> +	}
>> +
>> +	leaf1 = bp1->b_addr;
>> +	entry1 = &xfs_attr3_leaf_entryp(leaf1)[args->index];
>> +
>> +	leaf2 = bp2->b_addr;
>> +	entry2 = &xfs_attr3_leaf_entryp(leaf2)[args->index2];
>> +
>> +	*isflipped = (((entry1->flags & XFS_ATTR_INCOMPLETE) == 0) &&
> 
> Nit: ((entry1->flags & XFS_ATTR_INCOMPLETE) == 0) could be written as
> !(entry1->flags & XFS_ATTR_INCOMPLETE)
> 
>> +		      (entry2->flags & XFS_ATTR_INCOMPLETE));
>> +
>> +	xfs_trans_brelse(args->trans, bp1);
>> +	xfs_trans_brelse(args->trans, bp2);
> 
> This double-frees bp2 if bp1 == bp2.
Ok, will fix if needed.  Thanks for the review!  Sorry for the stray patch!

Allison
> 
> --D
> 
> 
>> +
>> +	return 0;
>> +}
>> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
>> index e108b37..12283cf 100644
>> --- a/fs/xfs/libxfs/xfs_attr_leaf.h
>> +++ b/fs/xfs/libxfs/xfs_attr_leaf.h
>> @@ -57,7 +57,9 @@ int	xfs_attr3_leaf_to_shortform(struct xfs_buf *bp,
>>   				   struct xfs_da_args *args, int forkoff);
>>   int	xfs_attr3_leaf_clearflag(struct xfs_da_args *args);
>>   int	xfs_attr3_leaf_setflag(struct xfs_da_args *args);
>> +int	xfs_attr3_leaf_flag_is_set(struct xfs_da_args *args, bool *isset);
>>   int	xfs_attr3_leaf_flipflags(struct xfs_da_args *args);
>> +int	xfs_attr3_leaf_flagsflipped(struct xfs_da_args *args, bool *isflipped);
>>   
>>   /*
>>    * Routines used for growing the Btree.
>> -- 
>> 2.7.4
>>