[PATCH] xfs: reset b_first_retry_time when clear the retry status of xfs_buf_t

[PATCH] xfs: reset b_first_retry_time when clear the retry status of xfs_buf_t

[Hou Tao]

After successful IO or permanent error, b_first_retry_time also
needs to be cleared, else the invalid first retry time will be
used by the next retry check.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 fs/xfs/xfs_buf_item.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 2975cb2..0306168 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1162,6 +1162,7 @@ xfs_buf_iodone_callbacks(
 	 */
 	bp->b_last_error = 0;
 	bp->b_retries = 0;
+	bp->b_first_retry_time = 0;
 
 	xfs_buf_do_callbacks(bp);
 	bp->b_fspriv = NULL;
-- 
2.5.0

Re: [PATCH] xfs: reset b_first_retry_time when clear the retry status of xfs_buf_t

[Brian Foster]

On Fri, Feb 03, 2017 at 05:08:25PM +0800, Hou Tao wrote:
> After successful IO or permanent error, b_first_retry_time also
> needs to be cleared, else the invalid first retry time will be
> used by the next retry check.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_buf_item.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 2975cb2..0306168 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1162,6 +1162,7 @@ xfs_buf_iodone_callbacks(
>  	 */
>  	bp->b_last_error = 0;
>  	bp->b_retries = 0;
> +	bp->b_first_retry_time = 0;
>  
>  	xfs_buf_do_callbacks(bp);
>  	bp->b_fspriv = NULL;
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: reset b_first_retry_time when clear the retry status of xfs_buf_t

[Darrick J. Wong]

On Fri, Feb 03, 2017 at 05:08:25PM +0800, Hou Tao wrote:
> After successful IO or permanent error, b_first_retry_time also
> needs to be cleared, else the invalid first retry time will be
> used by the next retry check.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  fs/xfs/xfs_buf_item.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 2975cb2..0306168 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1162,6 +1162,7 @@ xfs_buf_iodone_callbacks(
>  	 */
>  	bp->b_last_error = 0;
>  	bp->b_retries = 0;
> +	bp->b_first_retry_time = 0;

Looks ok, will try to queue it up as an rc7 fix.

--D

>  
>  	xfs_buf_do_callbacks(bp);
>  	bp->b_fspriv = NULL;
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: reset b_first_retry_time when clear the retry status of xfs_buf_t

[Dave Chinner]

On Fri, Feb 03, 2017 at 12:32:58PM -0800, Darrick J. Wong wrote:
> On Fri, Feb 03, 2017 at 05:08:25PM +0800, Hou Tao wrote:
> > After successful IO or permanent error, b_first_retry_time also
> > needs to be cleared, else the invalid first retry time will be
> > used by the next retry check.
> > 
> > Signed-off-by: Hou Tao <houtao1@huawei.com>
> > ---
> >  fs/xfs/xfs_buf_item.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 2975cb2..0306168 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -1162,6 +1162,7 @@ xfs_buf_iodone_callbacks(
> >  	 */
> >  	bp->b_last_error = 0;
> >  	bp->b_retries = 0;
> > +	bp->b_first_retry_time = 0;
> 
> Looks ok, will try to queue it up as an rc7 fix.

It's not a critical bug, nor is it a regression introduced in
4.10-rc1, so IMO it's not a -rc7 candidate fix. Just add it in the
for-next queue and mark it for stable. It'll still get to most users
at pretty much the same time via the stable trees...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: reset b_first_retry_time when clear the retry status of xfs_buf_t

[Darrick J. Wong]

On Sat, Feb 04, 2017 at 09:36:30AM +1100, Dave Chinner wrote:
> On Fri, Feb 03, 2017 at 12:32:58PM -0800, Darrick J. Wong wrote:
> > On Fri, Feb 03, 2017 at 05:08:25PM +0800, Hou Tao wrote:
> > > After successful IO or permanent error, b_first_retry_time also
> > > needs to be cleared, else the invalid first retry time will be
> > > used by the next retry check.
> > > 
> > > Signed-off-by: Hou Tao <houtao1@huawei.com>
> > > ---
> > >  fs/xfs/xfs_buf_item.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > > index 2975cb2..0306168 100644
> > > --- a/fs/xfs/xfs_buf_item.c
> > > +++ b/fs/xfs/xfs_buf_item.c
> > > @@ -1162,6 +1162,7 @@ xfs_buf_iodone_callbacks(
> > >  	 */
> > >  	bp->b_last_error = 0;
> > >  	bp->b_retries = 0;
> > > +	bp->b_first_retry_time = 0;
> > 
> > Looks ok, will try to queue it up as an rc7 fix.
> 
> It's not a critical bug, nor is it a regression introduced in
> 4.10-rc1, so IMO it's not a -rc7 candidate fix. Just add it in the
> for-next queue and mark it for stable. It'll still get to most users
> at pretty much the same time via the stable trees...

Ok.  I'll queue it up for 4.11 then.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: reset b_first_retry_time when clear the retry status of xfs_buf_t

[Darrick J. Wong]

On Fri, Feb 03, 2017 at 05:08:25PM +0800, Hou Tao wrote:
> After successful IO or permanent error, b_first_retry_time also
> needs to be cleared, else the invalid first retry time will be
> used by the next retry check.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  fs/xfs/xfs_buf_item.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 2975cb2..0306168 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1162,6 +1162,7 @@ xfs_buf_iodone_callbacks(
>  	 */
>  	bp->b_last_error = 0;
>  	bp->b_retries = 0;
> +	bp->b_first_retry_time = 0;

By the way, is there a testcase for this?  It looks like it shouldn't be
hard to generate a test that sets a timeout and interleaves temporary
failure and success.

--D

>  
>  	xfs_buf_do_callbacks(bp);
>  	bp->b_fspriv = NULL;
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: reset b_first_retry_time when clear the retry status of xfs_buf_t

[Hou Tao]

On 2017/2/4 7:17, Darrick J. Wong wrote:
> On Fri, Feb 03, 2017 at 05:08:25PM +0800, Hou Tao wrote:
>> After successful IO or permanent error, b_first_retry_time also
>> needs to be cleared, else the invalid first retry time will be
>> used by the next retry check.
>>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  fs/xfs/xfs_buf_item.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
>> index 2975cb2..0306168 100644
>> --- a/fs/xfs/xfs_buf_item.c
>> +++ b/fs/xfs/xfs_buf_item.c
>> @@ -1162,6 +1162,7 @@ xfs_buf_iodone_callbacks(
>>  	 */
>>  	bp->b_last_error = 0;
>>  	bp->b_retries = 0;
>> +	bp->b_first_retry_time = 0;
> 
> By the way, is there a testcase for this?  It looks like it shouldn't be
> hard to generate a test that sets a timeout and interleaves temporary
> failure and success.
> 
> --D

Not yet. There has been a test case (tests/xfs/264) for XFS error handler in xfstests,
and maybe I could add some interleaved failure-success test cases for both max_retries
and retry_timeout_seconds.

Regards,

Tao