| Message ID | 1486112905-38025-1-git-send-email-houtao1@huawei.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 -- 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
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 -- 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
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.
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 -- 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
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 -- 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
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 -- 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
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;
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(+)