diff mbox series

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

Message ID 20200223020611.1802-6-allison.henderson@oracle.com (mailing list archive)
State Superseded
Headers show
Series xfs: Delayed Ready Attrs | expand

Commit Message

Allison Henderson Feb. 23, 2020, 2:05 a.m. UTC 
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.

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

Comments

Chandan Rajendra Feb. 25, 2020, 12:53 p.m. UTC | #1 
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>

> 
> 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.
>   */
>
Allison Henderson Feb. 26, 2020, 2:20 a.m. UTC | #2 
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.
>>    */
>>
> 
>
diff mbox series

Patch

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.
  */