Re: [PATCH v7 05/19] xfs: Factor out new helper functions xfs_attr_rmtval_set

Allison Collins
2020-02-26T02:20:37Z

On 2/25/20 5:53 AM, Chandan Rajendra wrote:
> On Sunday, February 23, 2020 7:35 AM Allison Collins wrote:
>> Break xfs_attr_rmtval_set into two helper functions xfs_attr_rmt_find_hole
>> and xfs_attr_rmtval_set_value. xfs_attr_rmtval_set rolls the transaction between the
>> helpers, but delayed operations cannot. We will use the helpers later when
>> constructing new delayed attribute routines.
>
> I don't see any logical errors.
>
> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
Great! Thank you!

Allison
>
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr_remote.c | 149 +++++++++++++++++++++++++---------------
>> 1 file changed, 92 insertions(+), 57 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>> index df8aca5..d1eee24 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>> @@ -440,32 +440,23 @@ xfs_attr_rmtval_get(
>> }
>>
>> /*
>> - * Write the value associated with an attribute into the out-of-line buffer
>> - * that we have defined for it.
>> + * Find a "hole" in the attribute address space large enough for us to drop the
>> + * new attribute's value into
>> */
>> -int
>> -xfs_attr_rmtval_set(
>> +STATIC int
>> +xfs_attr_rmt_find_hole(
>> struct xfs_da_args *args)
>> {
>> struct xfs_inode *dp = args->dp;
>> struct xfs_mount *mp = dp->i_mount;
>> - struct xfs_bmbt_irec map;
>> - xfs_dablk_t lblkno;
>> - xfs_fileoff_t lfileoff = 0;
>> - uint8_t *src = args->value;
>> - int blkcnt;
>> - int valuelen;
>> - int nmap;
>> int error;
>> - int offset = 0;
>> -
>> - trace_xfs_attr_rmtval_set(args);
>> + int blkcnt;
>> + xfs_fileoff_t lfileoff = 0;
>>
>> /*
>> - * Find a "hole" in the attribute address space large enough for
>> - * us to drop the new attribute's value into. Because CRC enable
>> - * attributes have headers, we can't just do a straight byte to FSB
>> - * conversion and have to take the header space into account.
>> + * Because CRC enable attributes have headers, we can't just do a
>> + * straight byte to FSB conversion and have to take the header space
>> + * into account.
>> */
>> blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen);
>> error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff,
>> @@ -473,48 +464,26 @@ xfs_attr_rmtval_set(
>> if (error)
>> return error;
>>
>> - args->rmtblkno = lblkno = (xfs_dablk_t)lfileoff;
>> + args->rmtblkno = (xfs_dablk_t)lfileoff;
>> args->rmtblkcnt = blkcnt;
>>
>> - /*
>> - * Roll through the "value", allocating blocks on disk as required.
>> - */
>> - while (blkcnt > 0) {
>> - /*
>> - * Allocate a single extent, up to the size of the value.
>> - *
>> - * Note that we have to consider this a data allocation as we
>> - * write the remote attribute without logging the contents.
>> - * Hence we must ensure that we aren't using blocks that are on
>> - * the busy list so that we don't overwrite blocks which have
>> - * recently been freed but their transactions are not yet
>> - * committed to disk. If we overwrite the contents of a busy
>> - * extent and then crash then the block may not contain the
>> - * correct metadata after log recovery occurs.
>> - */
>> - nmap = 1;
>> - error = xfs_bmapi_write(args->trans, dp, (xfs_fileoff_t)lblkno,
>> - blkcnt, XFS_BMAPI_ATTRFORK, args->total, &map,
>> - &nmap);
>> - if (error)
>> - return error;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - return error;
>> -
>> - ASSERT(nmap == 1);
>> - ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
>> - (map.br_startblock != HOLESTARTBLOCK));
>> - lblkno += map.br_blockcount;
>> - blkcnt -= map.br_blockcount;
>> + return 0;
>> +}
>>
>> - /*
>> - * Start the next trans in the chain.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, dp);
>> - if (error)
>> - return error;
>> - }
>> +STATIC int
>> +xfs_attr_rmtval_set_value(
>> + struct xfs_da_args *args)
>> +{
>> + struct xfs_inode *dp = args->dp;
>> + struct xfs_mount *mp = dp->i_mount;
>> + struct xfs_bmbt_irec map;
>> + xfs_dablk_t lblkno;
>> + uint8_t *src = args->value;
>> + int blkcnt;
>> + int valuelen;
>> + int nmap;
>> + int error;
>> + int offset = 0;
>>
>> /*
>> * Roll through the "value", copying the attribute value to the
>> @@ -595,6 +564,72 @@ xfs_attr_rmtval_stale(
>> }
>>
>> /*
>> + * Write the value associated with an attribute into the out-of-line buffer
>> + * that we have defined for it.
>> + */
>> +int
>> +xfs_attr_rmtval_set(
>> + struct xfs_da_args *args)
>> +{
>> + struct xfs_inode *dp = args->dp;
>> + struct xfs_bmbt_irec map;
>> + xfs_dablk_t lblkno;
>> + int blkcnt;
>> + int nmap;
>> + int error;
>> +
>> + trace_xfs_attr_rmtval_set(args);
>> +
>> + error = xfs_attr_rmt_find_hole(args);
>> + if (error)
>> + return error;
>> +
>> + blkcnt = args->rmtblkcnt;
>> + lblkno = (xfs_dablk_t)args->rmtblkno;
>> + /*
>> + * Roll through the "value", allocating blocks on disk as required.
>> + */
>> + while (blkcnt > 0) {
>> + /*
>> + * Allocate a single extent, up to the size of the value.
>> + *
>> + * Note that we have to consider this a data allocation as we
>> + * write the remote attribute without logging the contents.
>> + * Hence we must ensure that we aren't using blocks that are on
>> + * the busy list so that we don't overwrite blocks which have
>> + * recently been freed but their transactions are not yet
>> + * committed to disk. If we overwrite the contents of a busy
>> + * extent and then crash then the block may not contain the
>> + * correct metadata after log recovery occurs.
>> + */
>> + nmap = 1;
>> + error = xfs_bmapi_write(args->trans, dp, (xfs_fileoff_t)lblkno,
>> + blkcnt, XFS_BMAPI_ATTRFORK, args->total, &map,
>> + &nmap);
>> + if (error)
>> + return error;
>> + error = xfs_defer_finish(&args->trans);
>> + if (error)
>> + return error;
>> +
>> + ASSERT(nmap == 1);
>> + ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
>> + (map.br_startblock != HOLESTARTBLOCK));
>> + lblkno += map.br_blockcount;
>> + blkcnt -= map.br_blockcount;
>> +
>> + /*
>> + * Start the next trans in the chain.
>> + */
>> + error = xfs_trans_roll_inode(&args->trans, dp);
>> + if (error)
>> + return error;
>> + }
>> +
>> + return xfs_attr_rmtval_set_value(args);
>> +}
>> +
>> +/*
>> * Remove the value associated with an attribute by deleting the
>> * out-of-line buffer that it is stored on.
>> */
>>
>
>