[PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc

[PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc

[Brian Foster]

The dquot allocation path in xfs_qm_dqread() currently uses the
attribute set log reservation, which appears to be incorrect. We
have reports of transaction reservation overruns with the current
code. E.g., a repeated run of xfstests test generic/270 on a 512b
block size fs occassionally produces the following in dmesg:

	XFS (sdN): xlog_write: reservation summary:
	  trans type  = QM_DQALLOC (30)
	  unit res    = 7080 bytes
	  current res = -632 bytes
	  total reg   = 0 bytes (o/flow = 0 bytes)
	  ophdrs      = 0 (ophdr space = 0 bytes)
	  ophdr + reg = 0 bytes
	  num regions = 0

	XFS (sdN): xlog_write: reservation ran out. Need to up reservation

The dquot allocation case should consist of a write reservation
(i.e., we are allocating a range of the internal quota file) plus
the size of the actual dquots. We already have a log reservation
definition for this operation (tr_qm_dqalloc). Use it in
xfs_qm_dqread() and update the log reservation calculation function
to use the write res. calculation function rather than reading the
assumed to be pre-calculated value directly.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
Hi all,

This issue was reported here:

https://bugzilla.redhat.com/show_bug.cgi?id=1052787

... and the patch seems to address the reservation overrun from my testing.
It also runs through an xfstests regression. Thanks.

Brian

 fs/xfs/xfs_dquot.c      | 2 +-
 fs/xfs/xfs_trans_resv.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 6b1e695..06280c6 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -614,7 +614,7 @@ xfs_qm_dqread(
 
 	if (flags & XFS_QMOPT_DQALLOC) {
 		tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
-		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm,
+		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc,
 					  XFS_QM_DQALLOC_SPACE_RES(mp), 0);
 		if (error)
 			goto error1;
diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
index 2fd59c0..60b7d40 100644
--- a/fs/xfs/xfs_trans_resv.c
+++ b/fs/xfs/xfs_trans_resv.c
@@ -651,8 +651,7 @@ STATIC uint
 xfs_calc_qm_dqalloc_reservation(
 	struct xfs_mount	*mp)
 {
-	ASSERT(M_RES(mp)->tr_write.tr_logres);
-	return M_RES(mp)->tr_write.tr_logres +
+	return xfs_calc_write_reservation(mp) +
 		xfs_calc_buf_res(1,
 			XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1);
 }
-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc

[Eric Sandeen]

On 1/17/14, 12:02 PM, Brian Foster wrote:
> The dquot allocation path in xfs_qm_dqread() currently uses the
> attribute set log reservation, which appears to be incorrect. We
> have reports of transaction reservation overruns with the current
> code. E.g., a repeated run of xfstests test generic/270 on a 512b
> block size fs occassionally produces the following in dmesg:
> 
> 	XFS (sdN): xlog_write: reservation summary:
> 	  trans type  = QM_DQALLOC (30)
> 	  unit res    = 7080 bytes
> 	  current res = -632 bytes
> 	  total reg   = 0 bytes (o/flow = 0 bytes)
> 	  ophdrs      = 0 (ophdr space = 0 bytes)
> 	  ophdr + reg = 0 bytes
> 	  num regions = 0
> 
> 	XFS (sdN): xlog_write: reservation ran out. Need to up reservation
> 
> The dquot allocation case should consist of a write reservation
> (i.e., we are allocating a range of the internal quota file) plus
> the size of the actual dquots. We already have a log reservation
> definition for this operation (tr_qm_dqalloc). Use it in
> xfs_qm_dqread() and update the log reservation calculation function
> to use the write res. calculation function rather than reading the
> assumed to be pre-calculated value directly.


good catch; I think there is at least one more issue in the patch that
refactored all of this:

-               if ((error = xfs_trans_reserve(tp, resblks,
-                               XFS_GROWRTALLOC_LOG_RES(mp), 0,
-                               XFS_TRANS_PERM_LOG_RES,
-                               XFS_DEFAULT_PERM_LOG_COUNT)))
+               error = xfs_trans_reserve(tp, &M_RES(mp)->tr_growdata,
+                                         resblks, 0);

shouldn't it be tr_growrtalloc ?

-Eric

> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> Hi all,
> 
> This issue was reported here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1052787
> 
> ... and the patch seems to address the reservation overrun from my testing.
> It also runs through an xfstests regression. Thanks.
> 
> Brian
> 
>  fs/xfs/xfs_dquot.c      | 2 +-
>  fs/xfs/xfs_trans_resv.c | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 6b1e695..06280c6 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -614,7 +614,7 @@ xfs_qm_dqread(
>  
>  	if (flags & XFS_QMOPT_DQALLOC) {
>  		tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
> -		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm,
> +		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc,
>  					  XFS_QM_DQALLOC_SPACE_RES(mp), 0);
>  		if (error)
>  			goto error1;
> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
> index 2fd59c0..60b7d40 100644
> --- a/fs/xfs/xfs_trans_resv.c
> +++ b/fs/xfs/xfs_trans_resv.c
> @@ -651,8 +651,7 @@ STATIC uint
>  xfs_calc_qm_dqalloc_reservation(
>  	struct xfs_mount	*mp)
>  {
> -	ASSERT(M_RES(mp)->tr_write.tr_logres);
> -	return M_RES(mp)->tr_write.tr_logres +
> +	return xfs_calc_write_reservation(mp) +
>  		xfs_calc_buf_res(1,
>  			XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1);
>  }
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc

[Brian Foster]

On 01/17/2014 01:12 PM, Eric Sandeen wrote:
> On 1/17/14, 12:02 PM, Brian Foster wrote:
>> The dquot allocation path in xfs_qm_dqread() currently uses the
>> attribute set log reservation, which appears to be incorrect. We
>> have reports of transaction reservation overruns with the current
>> code. E.g., a repeated run of xfstests test generic/270 on a 512b
>> block size fs occassionally produces the following in dmesg:
>>
>> 	XFS (sdN): xlog_write: reservation summary:
>> 	  trans type  = QM_DQALLOC (30)
>> 	  unit res    = 7080 bytes
>> 	  current res = -632 bytes
>> 	  total reg   = 0 bytes (o/flow = 0 bytes)
>> 	  ophdrs      = 0 (ophdr space = 0 bytes)
>> 	  ophdr + reg = 0 bytes
>> 	  num regions = 0
>>
>> 	XFS (sdN): xlog_write: reservation ran out. Need to up reservation
>>
>> The dquot allocation case should consist of a write reservation
>> (i.e., we are allocating a range of the internal quota file) plus
>> the size of the actual dquots. We already have a log reservation
>> definition for this operation (tr_qm_dqalloc). Use it in
>> xfs_qm_dqread() and update the log reservation calculation function
>> to use the write res. calculation function rather than reading the
>> assumed to be pre-calculated value directly.
> 
> 
> good catch; I think there is at least one more issue in the patch that
> refactored all of this:
> 
> -               if ((error = xfs_trans_reserve(tp, resblks,
> -                               XFS_GROWRTALLOC_LOG_RES(mp), 0,
> -                               XFS_TRANS_PERM_LOG_RES,
> -                               XFS_DEFAULT_PERM_LOG_COUNT)))
> +               error = xfs_trans_reserve(tp, &M_RES(mp)->tr_growdata,
> +                                         resblks, 0);
> 
> shouldn't it be tr_growrtalloc ?
> 

Yep, looks like it. XFS_GROWRTALLOC_LOG_RES() maps to tr_growrtalloc
prior to that commit (3d3c8b5222). Thanks for catching that, I'll fire
another one off shortly...

Brian

> -Eric
> 
>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>> ---
>> Hi all,
>>
>> This issue was reported here:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1052787
>>
>> ... and the patch seems to address the reservation overrun from my testing.
>> It also runs through an xfstests regression. Thanks.
>>
>> Brian
>>
>>  fs/xfs/xfs_dquot.c      | 2 +-
>>  fs/xfs/xfs_trans_resv.c | 3 +--
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
>> index 6b1e695..06280c6 100644
>> --- a/fs/xfs/xfs_dquot.c
>> +++ b/fs/xfs/xfs_dquot.c
>> @@ -614,7 +614,7 @@ xfs_qm_dqread(
>>  
>>  	if (flags & XFS_QMOPT_DQALLOC) {
>>  		tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
>> -		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm,
>> +		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc,
>>  					  XFS_QM_DQALLOC_SPACE_RES(mp), 0);
>>  		if (error)
>>  			goto error1;
>> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
>> index 2fd59c0..60b7d40 100644
>> --- a/fs/xfs/xfs_trans_resv.c
>> +++ b/fs/xfs/xfs_trans_resv.c
>> @@ -651,8 +651,7 @@ STATIC uint
>>  xfs_calc_qm_dqalloc_reservation(
>>  	struct xfs_mount	*mp)
>>  {
>> -	ASSERT(M_RES(mp)->tr_write.tr_logres);
>> -	return M_RES(mp)->tr_write.tr_logres +
>> +	return xfs_calc_write_reservation(mp) +
>>  		xfs_calc_buf_res(1,
>>  			XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1);
>>  }
>>
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc

[Jeff Liu]

On 01/18 2014 02:02 AM, Brian Foster wrote:
> The dquot allocation path in xfs_qm_dqread() currently uses the
> attribute set log reservation, which appears to be incorrect. We
> have reports of transaction reservation overruns with the current
> code. E.g., a repeated run of xfstests test generic/270 on a 512b
> block size fs occassionally produces the following in dmesg:
> 
> 	XFS (sdN): xlog_write: reservation summary:
> 	  trans type  = QM_DQALLOC (30)
> 	  unit res    = 7080 bytes
> 	  current res = -632 bytes
> 	  total reg   = 0 bytes (o/flow = 0 bytes)
> 	  ophdrs      = 0 (ophdr space = 0 bytes)
> 	  ophdr + reg = 0 bytes
> 	  num regions = 0
> 
> 	XFS (sdN): xlog_write: reservation ran out. Need to up reservation
> 
> The dquot allocation case should consist of a write reservation
> (i.e., we are allocating a range of the internal quota file) plus
> the size of the actual dquots. We already have a log reservation
> definition for this operation (tr_qm_dqalloc). Use it in
> xfs_qm_dqread() and update the log reservation calculation function
> to use the write res. calculation function rather than reading the
> assumed to be pre-calculated value directly.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> Hi all,
> 
> This issue was reported here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1052787
> 
> ... and the patch seems to address the reservation overrun from my testing.
> It also runs through an xfstests regression. Thanks.
> 
> Brian
> 
>  fs/xfs/xfs_dquot.c      | 2 +-
>  fs/xfs/xfs_trans_resv.c | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 6b1e695..06280c6 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -614,7 +614,7 @@ xfs_qm_dqread(
>  
>  	if (flags & XFS_QMOPT_DQALLOC) {
>  		tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
> -		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm,
> +		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc,
>  					  XFS_QM_DQALLOC_SPACE_RES(mp), 0);

Ah, another issue I introduced for refactoring xfs_trans_reserve() interface.
>  		if (error)
>  			goto error1;
> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
> index 2fd59c0..60b7d40 100644
> --- a/fs/xfs/xfs_trans_resv.c
> +++ b/fs/xfs/xfs_trans_resv.c
> @@ -651,8 +651,7 @@ STATIC uint
>  xfs_calc_qm_dqalloc_reservation(
>  	struct xfs_mount	*mp)
>  {
> -	ASSERT(M_RES(mp)->tr_write.tr_logres);
> -	return M_RES(mp)->tr_write.tr_logres +
> +	return xfs_calc_write_reservation(mp) +
>  		xfs_calc_buf_res(1,
>  			XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1);
>  }

I'm fine to call xfs_calc_write_reservation() rather than the current hard-code,
especially we can get rid of the ASSERT() though tr_write.tr_logres should not
be zero as it is initialized in advance at xfs_trans_resv_calc().

Just a little nit pick :), that would be better to consolidate the comments in
this fix as well, i.e, 
*	the write transaction log space: M_RES(mp)->tr_write.tr_logres.


Thanks,
-Jeff

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: use tr_qm_dqalloc log reservation for dquot alloc

[Brian Foster]

On 01/18/2014 09:17 AM, Jeff Liu wrote:
> On 01/18 2014 02:02 AM, Brian Foster wrote:
>> The dquot allocation path in xfs_qm_dqread() currently uses the
>> attribute set log reservation, which appears to be incorrect. We
>> have reports of transaction reservation overruns with the current
>> code. E.g., a repeated run of xfstests test generic/270 on a 512b
>> block size fs occassionally produces the following in dmesg:
>>
>> 	XFS (sdN): xlog_write: reservation summary:
>> 	  trans type  = QM_DQALLOC (30)
>> 	  unit res    = 7080 bytes
>> 	  current res = -632 bytes
>> 	  total reg   = 0 bytes (o/flow = 0 bytes)
>> 	  ophdrs      = 0 (ophdr space = 0 bytes)
>> 	  ophdr + reg = 0 bytes
>> 	  num regions = 0
>>
>> 	XFS (sdN): xlog_write: reservation ran out. Need to up reservation
>>
>> The dquot allocation case should consist of a write reservation
>> (i.e., we are allocating a range of the internal quota file) plus
>> the size of the actual dquots. We already have a log reservation
>> definition for this operation (tr_qm_dqalloc). Use it in
>> xfs_qm_dqread() and update the log reservation calculation function
>> to use the write res. calculation function rather than reading the
>> assumed to be pre-calculated value directly.
>>
>> Signed-off-by: Brian Foster <bfoster@redhat.com>
>> ---
>> Hi all,
>>
>> This issue was reported here:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1052787
>>
>> ... and the patch seems to address the reservation overrun from my testing.
>> It also runs through an xfstests regression. Thanks.
>>
>> Brian
>>
>>  fs/xfs/xfs_dquot.c      | 2 +-
>>  fs/xfs/xfs_trans_resv.c | 3 +--
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
>> index 6b1e695..06280c6 100644
>> --- a/fs/xfs/xfs_dquot.c
>> +++ b/fs/xfs/xfs_dquot.c
>> @@ -614,7 +614,7 @@ xfs_qm_dqread(
>>  
>>  	if (flags & XFS_QMOPT_DQALLOC) {
>>  		tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
>> -		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_attrsetm,
>> +		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_dqalloc,
>>  					  XFS_QM_DQALLOC_SPACE_RES(mp), 0);
> 
> Ah, another issue I introduced for refactoring xfs_trans_reserve() interface.
>>  		if (error)
>>  			goto error1;
>> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
>> index 2fd59c0..60b7d40 100644
>> --- a/fs/xfs/xfs_trans_resv.c
>> +++ b/fs/xfs/xfs_trans_resv.c
>> @@ -651,8 +651,7 @@ STATIC uint
>>  xfs_calc_qm_dqalloc_reservation(
>>  	struct xfs_mount	*mp)
>>  {
>> -	ASSERT(M_RES(mp)->tr_write.tr_logres);
>> -	return M_RES(mp)->tr_write.tr_logres +
>> +	return xfs_calc_write_reservation(mp) +
>>  		xfs_calc_buf_res(1,
>>  			XFS_FSB_TO_B(mp, XFS_DQUOT_CLUSTER_SIZE_FSB) - 1);
>>  }
> 
> I'm fine to call xfs_calc_write_reservation() rather than the current hard-code,
> especially we can get rid of the ASSERT() though tr_write.tr_logres should not
> be zero as it is initialized in advance at xfs_trans_resv_calc().
> 

Right... I just decided to call the write calc size function because
that seems to be the standard operating procedure with the new code and
it eliminates the fact that this is subject to initialization order,
even if not a bug at the moment. :)

> Just a little nit pick :), that would be better to consolidate the comments in
> this fix as well, i.e, 
> *	the write transaction log space: M_RES(mp)->tr_write.tr_logres.
> 

Ok, I'll send a new patch that cleans up the comment. Thanks for review.

Brian

> 
> Thanks,
> -Jeff
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs