| Message ID | 20210402094937.4072606-2-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: testcases for shrinking free space in the last AG | expand |
On Fri, Apr 02, 2021 at 05:49:35PM +0800, Gao Xiang wrote: > In order to detect whether the current kernel supports XFS shrinking. > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > common/xfs | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/common/xfs b/common/xfs > index 69f76d6e..c6c2e3f5 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -766,6 +766,20 @@ _require_xfs_mkfs_without_validation() > fi > } > > +_require_xfs_scratch_shrink() > +{ > + _require_scratch > + _require_command "$XFS_GROWFS_PROG" xfs_growfs > + > + _scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs >/dev/null > + . $tmp.mkfs > + _scratch_mount > + # here just to check if kernel supports, no need do more extra work > + $XFS_GROWFS_PROG -D$((dblocks-1)) "$SCRATCH_MNT" > /dev/null 2>&1 || \ > + _notrun "kernel does not support shrinking" I think isn't sufficiently precise -- if xfs_growfs (userspace) doesn't support shrinking it'll error out with "data size XXX too small", and if the kernel doesn't support shrink, it'll return EINVAL. As written, this code attempts a single-block shrink and disables the entire test if that fails for any reason, even if that reason is that the last block in the filesystem isn't free, or we ran out of memory, or something like that. I think this needs to check the output of xfs_growfs to make the decision to _notrun. --D > + _scratch_unmount > +} > + > # XFS ability to change UUIDs on V5/CRC filesystems > # > _require_meta_uuid() > -- > 2.27.0 >
On Mon, May 10, 2021 at 10:59:52AM -0700, Darrick J. Wong wrote: > On Fri, Apr 02, 2021 at 05:49:35PM +0800, Gao Xiang wrote: > > In order to detect whether the current kernel supports XFS shrinking. > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > --- > > common/xfs | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/common/xfs b/common/xfs > > index 69f76d6e..c6c2e3f5 100644 > > --- a/common/xfs > > +++ b/common/xfs > > @@ -766,6 +766,20 @@ _require_xfs_mkfs_without_validation() > > fi > > } > > > > +_require_xfs_scratch_shrink() > > +{ > > + _require_scratch > > + _require_command "$XFS_GROWFS_PROG" xfs_growfs > > + > > + _scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs >/dev/null > > + . $tmp.mkfs > > + _scratch_mount > > + # here just to check if kernel supports, no need do more extra work > > + $XFS_GROWFS_PROG -D$((dblocks-1)) "$SCRATCH_MNT" > /dev/null 2>&1 || \ > > + _notrun "kernel does not support shrinking" > > I think isn't sufficiently precise -- if xfs_growfs (userspace) doesn't > support shrinking it'll error out with "data size XXX too small", and if > the kernel doesn't support shrink, it'll return EINVAL. I'm not sure if we need to identify such 2 cases (xfsprogs doesn't support and/or kernel doesn't support), but if it's really needed I think I could update it. But I've confirmed with testing that both two cases can be handled with the statements above properly. > > As written, this code attempts a single-block shrink and disables the > entire test if that fails for any reason, even if that reason is that > the last block in the filesystem isn't free, or we ran out of memory, or > something like that. hmm... the filesystem here is brandly new, I think at least it'd be considered as "the last block in the new filesystem is free". If we're worried that such promise could be broken, I think some other golden output is unstable as well (although unrelated to this.) By that time, I think the test script should be updated then instead. Or am I missing something? If we're worried about runing out of memory, I think the whole xfstests could not be predictable. I'm not sure if we need to handle such case. > > I think this needs to check the output of xfs_growfs to make the > decision to _notrun. I could check some golden output such as "data size XXX too small", yet I still don't think we should check some cases e.g. run out of memory.. Thanks, Gao Xiang > > --D > > > + _scratch_unmount > > +} > > + > > # XFS ability to change UUIDs on V5/CRC filesystems > > # > > _require_meta_uuid() > > -- > > 2.27.0 > > >
On Tue, May 11, 2021 at 10:02:48AM +0800, Gao Xiang wrote: > On Mon, May 10, 2021 at 10:59:52AM -0700, Darrick J. Wong wrote: > > On Fri, Apr 02, 2021 at 05:49:35PM +0800, Gao Xiang wrote: > > > In order to detect whether the current kernel supports XFS shrinking. > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > > --- > > > common/xfs | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/common/xfs b/common/xfs > > > index 69f76d6e..c6c2e3f5 100644 > > > --- a/common/xfs > > > +++ b/common/xfs > > > @@ -766,6 +766,20 @@ _require_xfs_mkfs_without_validation() > > > fi > > > } > > > > > > +_require_xfs_scratch_shrink() > > > +{ > > > + _require_scratch > > > + _require_command "$XFS_GROWFS_PROG" xfs_growfs > > > + > > > + _scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs >/dev/null > > > + . $tmp.mkfs > > > + _scratch_mount > > > + # here just to check if kernel supports, no need do more extra work > > > + $XFS_GROWFS_PROG -D$((dblocks-1)) "$SCRATCH_MNT" > /dev/null 2>&1 || \ > > > + _notrun "kernel does not support shrinking" > > > > I think isn't sufficiently precise -- if xfs_growfs (userspace) doesn't > > support shrinking it'll error out with "data size XXX too small", and if > > the kernel doesn't support shrink, it'll return EINVAL. > > I'm not sure if we need to identify such 2 cases (xfsprogs doesn't support > and/or kernel doesn't support), but if it's really needed I think I could > update it. But I've confirmed with testing that both two cases can be > handled with the statements above properly. > > > > > As written, this code attempts a single-block shrink and disables the > > entire test if that fails for any reason, even if that reason is that > > the last block in the filesystem isn't free, or we ran out of memory, or > > something like that. > > hmm... the filesystem here is brandly new, I think at least it'd be > considered as "the last block in the new filesystem is free". If we're > worried that such promise could be broken, I think some other golden > output is unstable as well (although unrelated to this.) By that time, > I think the test script should be updated then instead. Or am I missing > something? > > If we're worried about runing out of memory, I think the whole xfstests > could not be predictable. I'm not sure if we need to handle such case. I'm not specifically worried about running out of memory, I'm mostly worried that some /other/ implementation bug (or disk format variation) will show up and triggers the _notrun, and nobody will notice that the shrink tests quietly stop running. --D > > > > I think this needs to check the output of xfs_growfs to make the > > decision to _notrun. > > I could check some golden output such as "data size XXX too small", yet > I still don't think we should check some cases e.g. run out of memory.. > > Thanks, > Gao Xiang > > > > > --D > > > > > + _scratch_unmount > > > +} > > > + > > > # XFS ability to change UUIDs on V5/CRC filesystems > > > # > > > _require_meta_uuid() > > > -- > > > 2.27.0 > > > > > >
diff --git a/common/xfs b/common/xfs index 69f76d6e..c6c2e3f5 100644 --- a/common/xfs +++ b/common/xfs @@ -766,6 +766,20 @@ _require_xfs_mkfs_without_validation() fi } +_require_xfs_scratch_shrink() +{ + _require_scratch + _require_command "$XFS_GROWFS_PROG" xfs_growfs + + _scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs >/dev/null + . $tmp.mkfs + _scratch_mount + # here just to check if kernel supports, no need do more extra work + $XFS_GROWFS_PROG -D$((dblocks-1)) "$SCRATCH_MNT" > /dev/null 2>&1 || \ + _notrun "kernel does not support shrinking" + _scratch_unmount +} + # XFS ability to change UUIDs on V5/CRC filesystems # _require_meta_uuid()
In order to detect whether the current kernel supports XFS shrinking. Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- common/xfs | 14 ++++++++++++++ 1 file changed, 14 insertions(+)