diff mbox series

[v6,7/7] xfs: add error injection for per-AG resv failure when shrinkfs

Message ID 20210126125621.3846735-8-hsiangkao@redhat.com (mailing list archive)
State Superseded
Headers show
Series xfs: support shrinking free space in the last AG | expand

Commit Message

Gao Xiang Jan. 26, 2021, 12:56 p.m. UTC 
per-AG resv failure after fixing up freespace is hard to test in an
effective way, so directly add an error injection path to observe
such error handling path works as expected.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.c       | 5 +++++
 fs/xfs/libxfs/xfs_errortag.h | 4 +++-
 fs/xfs/xfs_error.c           | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

Comments

Brian Foster Feb. 3, 2021, 2:23 p.m. UTC | #1 
On Tue, Jan 26, 2021 at 08:56:21PM +0800, Gao Xiang wrote:
> per-AG resv failure after fixing up freespace is hard to test in an
> effective way, so directly add an error injection path to observe
> such error handling path works as expected.
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag.c       | 5 +++++
>  fs/xfs/libxfs/xfs_errortag.h | 4 +++-
>  fs/xfs/xfs_error.c           | 2 ++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index c6e68e265269..5076913c153f 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -23,6 +23,7 @@
>  #include "xfs_ag_resv.h"
>  #include "xfs_health.h"
>  #include "xfs_error.h"
> +#include "xfs_errortag.h"
>  #include "xfs_bmap.h"
>  #include "xfs_defer.h"
>  #include "xfs_log_format.h"
> @@ -559,6 +560,10 @@ xfs_ag_shrink_space(
>  	be32_add_cpu(&agf->agf_length, -len);
>  
>  	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> +
> +	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL))
> +		err2 = -ENOSPC;
> +

Seems reasonable, but I feel like this could be broadened to serve as a
generic perag reservation error tag. I suppose we might not be able to
use it on a clean mount, but perhaps it could be reused for growfs and
remount. Hm?

Brian

>  	if (err2) {
>  		be32_add_cpu(&agi->agi_length, len);
>  		be32_add_cpu(&agf->agf_length, len);
> diff --git a/fs/xfs/libxfs/xfs_errortag.h b/fs/xfs/libxfs/xfs_errortag.h
> index 6ca9084b6934..5fd71a930b68 100644
> --- a/fs/xfs/libxfs/xfs_errortag.h
> +++ b/fs/xfs/libxfs/xfs_errortag.h
> @@ -40,6 +40,7 @@
>  #define XFS_ERRTAG_REFCOUNT_FINISH_ONE			25
>  #define XFS_ERRTAG_BMAP_FINISH_ONE			26
>  #define XFS_ERRTAG_AG_RESV_CRITICAL			27
> +
>  /*
>   * DEBUG mode instrumentation to test and/or trigger delayed allocation
>   * block killing in the event of failed writes. When enabled, all
> @@ -58,7 +59,8 @@
>  #define XFS_ERRTAG_BUF_IOERROR				35
>  #define XFS_ERRTAG_REDUCE_MAX_IEXTENTS			36
>  #define XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT		37
> -#define XFS_ERRTAG_MAX					38
> +#define XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL		38
> +#define XFS_ERRTAG_MAX					39
>  
>  /*
>   * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 185b4915b7bf..7bae34bfddd2 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -168,6 +168,7 @@ XFS_ERRORTAG_ATTR_RW(iunlink_fallback,	XFS_ERRTAG_IUNLINK_FALLBACK);
>  XFS_ERRORTAG_ATTR_RW(buf_ioerror,	XFS_ERRTAG_BUF_IOERROR);
>  XFS_ERRORTAG_ATTR_RW(reduce_max_iextents,	XFS_ERRTAG_REDUCE_MAX_IEXTENTS);
>  XFS_ERRORTAG_ATTR_RW(bmap_alloc_minlen_extent,	XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT);
> +XFS_ERRORTAG_ATTR_RW(shrinkfs_ag_resv_fail, XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL);
>  
>  static struct attribute *xfs_errortag_attrs[] = {
>  	XFS_ERRORTAG_ATTR_LIST(noerror),
> @@ -208,6 +209,7 @@ static struct attribute *xfs_errortag_attrs[] = {
>  	XFS_ERRORTAG_ATTR_LIST(buf_ioerror),
>  	XFS_ERRORTAG_ATTR_LIST(reduce_max_iextents),
>  	XFS_ERRORTAG_ATTR_LIST(bmap_alloc_minlen_extent),
> +	XFS_ERRORTAG_ATTR_LIST(shrinkfs_ag_resv_fail),
>  	NULL,
>  };
>  
> -- 
> 2.27.0
>
Gao Xiang Feb. 3, 2021, 3:01 p.m. UTC | #2 
Hi Brian,

On Wed, Feb 03, 2021 at 09:23:59AM -0500, Brian Foster wrote:
> On Tue, Jan 26, 2021 at 08:56:21PM +0800, Gao Xiang wrote:
> > per-AG resv failure after fixing up freespace is hard to test in an
> > effective way, so directly add an error injection path to observe
> > such error handling path works as expected.
> > 
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_ag.c       | 5 +++++
> >  fs/xfs/libxfs/xfs_errortag.h | 4 +++-
> >  fs/xfs/xfs_error.c           | 2 ++
> >  3 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index c6e68e265269..5076913c153f 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -23,6 +23,7 @@
> >  #include "xfs_ag_resv.h"
> >  #include "xfs_health.h"
> >  #include "xfs_error.h"
> > +#include "xfs_errortag.h"
> >  #include "xfs_bmap.h"
> >  #include "xfs_defer.h"
> >  #include "xfs_log_format.h"
> > @@ -559,6 +560,10 @@ xfs_ag_shrink_space(
> >  	be32_add_cpu(&agf->agf_length, -len);
> >  
> >  	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> > +
> > +	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL))
> > +		err2 = -ENOSPC;
> > +
> 
> Seems reasonable, but I feel like this could be broadened to serve as a
> generic perag reservation error tag. I suppose we might not be able to
> use it on a clean mount, but perhaps it could be reused for growfs and
> remount. Hm?

I think it could be done in that way, yet currently the logic is just to
verify the shrink error handling case above rather than extend to actually
error inject per-AG reservation for now... I could rename the errortag
for later reuse (some better naming? I'm not good at this...) in advance
yet real per-AG reservation error injection might be more complicated
than just error out with -ENOSPC, and it's somewhat out of scope of this
patchset for now...

Thanks,
Gao Xiang

> 
> Brian
Brian Foster Feb. 3, 2021, 6:01 p.m. UTC | #3 
On Wed, Feb 03, 2021 at 11:01:32PM +0800, Gao Xiang wrote:
> Hi Brian,
> 
> On Wed, Feb 03, 2021 at 09:23:59AM -0500, Brian Foster wrote:
> > On Tue, Jan 26, 2021 at 08:56:21PM +0800, Gao Xiang wrote:
> > > per-AG resv failure after fixing up freespace is hard to test in an
> > > effective way, so directly add an error injection path to observe
> > > such error handling path works as expected.
> > > 
> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_ag.c       | 5 +++++
> > >  fs/xfs/libxfs/xfs_errortag.h | 4 +++-
> > >  fs/xfs/xfs_error.c           | 2 ++
> > >  3 files changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > > index c6e68e265269..5076913c153f 100644
> > > --- a/fs/xfs/libxfs/xfs_ag.c
> > > +++ b/fs/xfs/libxfs/xfs_ag.c
> > > @@ -23,6 +23,7 @@
> > >  #include "xfs_ag_resv.h"
> > >  #include "xfs_health.h"
> > >  #include "xfs_error.h"
> > > +#include "xfs_errortag.h"
> > >  #include "xfs_bmap.h"
> > >  #include "xfs_defer.h"
> > >  #include "xfs_log_format.h"
> > > @@ -559,6 +560,10 @@ xfs_ag_shrink_space(
> > >  	be32_add_cpu(&agf->agf_length, -len);
> > >  
> > >  	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> > > +
> > > +	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL))
> > > +		err2 = -ENOSPC;
> > > +
> > 
> > Seems reasonable, but I feel like this could be broadened to serve as a
> > generic perag reservation error tag. I suppose we might not be able to
> > use it on a clean mount, but perhaps it could be reused for growfs and
> > remount. Hm?
> 
> I think it could be done in that way, yet currently the logic is just to
> verify the shrink error handling case above rather than extend to actually
> error inject per-AG reservation for now... I could rename the errortag
> for later reuse (some better naming? I'm not good at this...) in advance
> yet real per-AG reservation error injection might be more complicated
> than just error out with -ENOSPC, and it's somewhat out of scope of this
> patchset for now...
> 

I don't think it needs to be any more complicated than the logic you
have here. Just bury it further down in in the perag res init code,
rename it to something like ERRTAG_AG_RESV_FAIL, and use it the exact
same way for shrink testing. For example, maybe drop it into
__xfs_ag_resv_init() near the xfs_mod_fdblocks() call so we can also
take advantage of the tracepoint that triggers on -ENOSPC for
informational purposes:

	error = xfs_mod_fdblocks(...);
	if (!error && XFS_TEST_ERROR(false, mp, XFS_ERRTAG_AG_RESV_FAIL))
		error = -ENOSPC;
	if (error) {
		...
	}

Brian

> Thanks,
> Gao Xiang
> 
> > 
> > Brian
>
Gao Xiang Feb. 4, 2021, 9:20 a.m. UTC | #4 
On Wed, Feb 03, 2021 at 01:01:40PM -0500, Brian Foster wrote:
> On Wed, Feb 03, 2021 at 11:01:32PM +0800, Gao Xiang wrote:

...

> > > > @@ -559,6 +560,10 @@ xfs_ag_shrink_space(
> > > >  	be32_add_cpu(&agf->agf_length, -len);
> > > >  
> > > >  	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> > > > +
> > > > +	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL))
> > > > +		err2 = -ENOSPC;
> > > > +
> > > 
> > > Seems reasonable, but I feel like this could be broadened to serve as a
> > > generic perag reservation error tag. I suppose we might not be able to
> > > use it on a clean mount, but perhaps it could be reused for growfs and
> > > remount. Hm?
> > 
> > I think it could be done in that way, yet currently the logic is just to
> > verify the shrink error handling case above rather than extend to actually
> > error inject per-AG reservation for now... I could rename the errortag
> > for later reuse (some better naming? I'm not good at this...) in advance
> > yet real per-AG reservation error injection might be more complicated
> > than just error out with -ENOSPC, and it's somewhat out of scope of this
> > patchset for now...
> > 
> 
> I don't think it needs to be any more complicated than the logic you
> have here. Just bury it further down in in the perag res init code,
> rename it to something like ERRTAG_AG_RESV_FAIL, and use it the exact
> same way for shrink testing. For example, maybe drop it into
> __xfs_ag_resv_init() near the xfs_mod_fdblocks() call so we can also
> take advantage of the tracepoint that triggers on -ENOSPC for
> informational purposes:
> 
> 	error = xfs_mod_fdblocks(...);
> 	if (!error && XFS_TEST_ERROR(false, mp, XFS_ERRTAG_AG_RESV_FAIL))
> 		error = -ENOSPC;
> 	if (error) {
> 		...
> 	}

Ok, I didn't look into much more about it since it's out of scope. I'd
try in the next version.

Thanks,
Gao Xiang

> 
> Brian
> 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > Brian
> > 
>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index c6e68e265269..5076913c153f 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -23,6 +23,7 @@ 
 #include "xfs_ag_resv.h"
 #include "xfs_health.h"
 #include "xfs_error.h"
+#include "xfs_errortag.h"
 #include "xfs_bmap.h"
 #include "xfs_defer.h"
 #include "xfs_log_format.h"
@@ -559,6 +560,10 @@  xfs_ag_shrink_space(
 	be32_add_cpu(&agf->agf_length, -len);
 
 	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
+
+	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL))
+		err2 = -ENOSPC;
+
 	if (err2) {
 		be32_add_cpu(&agi->agi_length, len);
 		be32_add_cpu(&agf->agf_length, len);
diff --git a/fs/xfs/libxfs/xfs_errortag.h b/fs/xfs/libxfs/xfs_errortag.h
index 6ca9084b6934..5fd71a930b68 100644
--- a/fs/xfs/libxfs/xfs_errortag.h
+++ b/fs/xfs/libxfs/xfs_errortag.h
@@ -40,6 +40,7 @@ 
 #define XFS_ERRTAG_REFCOUNT_FINISH_ONE			25
 #define XFS_ERRTAG_BMAP_FINISH_ONE			26
 #define XFS_ERRTAG_AG_RESV_CRITICAL			27
+
 /*
  * DEBUG mode instrumentation to test and/or trigger delayed allocation
  * block killing in the event of failed writes. When enabled, all
@@ -58,7 +59,8 @@ 
 #define XFS_ERRTAG_BUF_IOERROR				35
 #define XFS_ERRTAG_REDUCE_MAX_IEXTENTS			36
 #define XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT		37
-#define XFS_ERRTAG_MAX					38
+#define XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL		38
+#define XFS_ERRTAG_MAX					39
 
 /*
  * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 185b4915b7bf..7bae34bfddd2 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -168,6 +168,7 @@  XFS_ERRORTAG_ATTR_RW(iunlink_fallback,	XFS_ERRTAG_IUNLINK_FALLBACK);
 XFS_ERRORTAG_ATTR_RW(buf_ioerror,	XFS_ERRTAG_BUF_IOERROR);
 XFS_ERRORTAG_ATTR_RW(reduce_max_iextents,	XFS_ERRTAG_REDUCE_MAX_IEXTENTS);
 XFS_ERRORTAG_ATTR_RW(bmap_alloc_minlen_extent,	XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT);
+XFS_ERRORTAG_ATTR_RW(shrinkfs_ag_resv_fail, XFS_ERRTAG_SHRINKFS_AG_RESV_FAIL);
 
 static struct attribute *xfs_errortag_attrs[] = {
 	XFS_ERRORTAG_ATTR_LIST(noerror),
@@ -208,6 +209,7 @@  static struct attribute *xfs_errortag_attrs[] = {
 	XFS_ERRORTAG_ATTR_LIST(buf_ioerror),
 	XFS_ERRORTAG_ATTR_LIST(reduce_max_iextents),
 	XFS_ERRORTAG_ATTR_LIST(bmap_alloc_minlen_extent),
+	XFS_ERRORTAG_ATTR_LIST(shrinkfs_ag_resv_fail),
 	NULL,
 };