diff mbox series

[6/8] xfs/122: update test to pick up rtword/suminfo ondisk unions

Message ID 170899915304.896550.17104868811908659798.stgit@frogsfrogsfrogs (mailing list archive)
State New, archived
Headers show
Series [1/8] generic/604: try to make race occur reliably | expand

Commit Message

Darrick J. Wong Feb. 27, 2024, 2:02 a.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Update this test to check that the ondisk unions for rt bitmap word and
rt summary counts are always the correct size.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/122     |    2 +-
 tests/xfs/122.out |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Zorro Lang Feb. 27, 2024, 5:10 a.m. UTC | #1
On Mon, Feb 26, 2024 at 06:02:05PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Update this test to check that the ondisk unions for rt bitmap word and
> rt summary counts are always the correct size.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/xfs/122     |    2 +-
>  tests/xfs/122.out |    2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/tests/xfs/122 b/tests/xfs/122
> index ba927c77c4..4e5ba1dfee 100755
> --- a/tests/xfs/122
> +++ b/tests/xfs/122
> @@ -195,7 +195,7 @@ echo 'int main(int argc, char *argv[]) {' >>$cprog
>  #
>  cat /usr/include/xfs/xfs*.h | indent |\
>  _attribute_filter |\
> -grep -E '(} *xfs_.*_t|^struct xfs_[a-z0-9_]*$)' |\
> +grep -E '(} *xfs_.*_t|^(union|struct) xfs_[a-z0-9_]*$)' |\
>  grep -E -v -f $tmp.ignore |\
>  sed -e 's/^.*}[[:space:]]*//g' -e 's/;.*$//g' -e 's/_t, /_t\n/g' |\
>  sort | uniq |\
> diff --git a/tests/xfs/122.out b/tests/xfs/122.out
> index 067a0ec76b..a2b57cfb9b 100644
> --- a/tests/xfs/122.out
> +++ b/tests/xfs/122.out
> @@ -124,6 +124,8 @@ sizeof(struct xfs_swap_extent) = 64
>  sizeof(struct xfs_sxd_log_format) = 16
>  sizeof(struct xfs_sxi_log_format) = 80
>  sizeof(struct xfs_unmount_log_format) = 8
> +sizeof(union xfs_rtword_raw) = 4
> +sizeof(union xfs_suminfo_raw) = 4
>  sizeof(xfs_agf_t) = 224
>  sizeof(xfs_agfl_t) = 36
>  sizeof(xfs_agi_t) = 344
>
Christoph Hellwig Feb. 27, 2024, 2:54 p.m. UTC | #2
Can we please just kill the goddamn test?  Just waiting for the
xfsprogs 6.8 resync to submit the static_asserts for libxfs that
will handle this much better.
Darrick J. Wong Feb. 28, 2024, 1:27 a.m. UTC | #3
On Tue, Feb 27, 2024 at 06:54:41AM -0800, Christoph Hellwig wrote:
> Can we please just kill the goddamn test?  Just waiting for the
> xfsprogs 6.8 resync to submit the static_asserts for libxfs that
> will handle this much better.

I'll be very happen when we scuttle xfs/122 finally.

However, in theory it's still be useful for QA departments to make sure
that xfsprogs backports (HA!) don't accidentally break things.

IOWs, I advocate for _notrunning this test if xfsprogs >= 6.8 is
detected, not removing it completely.

Unless someone wants to chime in and say that actually, nobody backports
stuff to old xfsprogs?  (We don't really...)

--D
Christoph Hellwig Feb. 28, 2024, 3:39 p.m. UTC | #4
On Tue, Feb 27, 2024 at 05:27:04PM -0800, Darrick J. Wong wrote:
> On Tue, Feb 27, 2024 at 06:54:41AM -0800, Christoph Hellwig wrote:
> > Can we please just kill the goddamn test?  Just waiting for the
> > xfsprogs 6.8 resync to submit the static_asserts for libxfs that
> > will handle this much better.
> 
> I'll be very happen when we scuttle xfs/122 finally.
> 
> However, in theory it's still be useful for QA departments to make sure
> that xfsprogs backports (HA!) don't accidentally break things.
> 
> IOWs, I advocate for _notrunning this test if xfsprogs >= 6.8 is
> detected, not removing it completely.
> 
> Unless someone wants to chime in and say that actually, nobody backports
> stuff to old xfsprogs?  (We don't really...)

Well, who is going to backport changes to the on-disk format in a way
that is complex enough to change strutures, and not also backport the
patch to actually check the sizes?  Sounds like a weird use case to
optimize for.
Darrick J. Wong Feb. 29, 2024, 5:48 p.m. UTC | #5
On Wed, Feb 28, 2024 at 07:39:29AM -0800, Christoph Hellwig wrote:
> On Tue, Feb 27, 2024 at 05:27:04PM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 27, 2024 at 06:54:41AM -0800, Christoph Hellwig wrote:
> > > Can we please just kill the goddamn test?  Just waiting for the
> > > xfsprogs 6.8 resync to submit the static_asserts for libxfs that
> > > will handle this much better.
> > 
> > I'll be very happen when we scuttle xfs/122 finally.
> > 
> > However, in theory it's still be useful for QA departments to make sure
> > that xfsprogs backports (HA!) don't accidentally break things.
> > 
> > IOWs, I advocate for _notrunning this test if xfsprogs >= 6.8 is
> > detected, not removing it completely.
> > 
> > Unless someone wants to chime in and say that actually, nobody backports
> > stuff to old xfsprogs?  (We don't really...)
> 
> Well, who is going to backport changes to the on-disk format in a way
> that is complex enough to change strutures, and not also backport the
> patch to actually check the sizes?  Sounds like a weird use case to
> optimize for.

It turns out that xfs/122 also captures ioctl structure sizes, and those
are /not/ captured by xfs_ondisk.h.  I think we should add those before
we kill xfs/122.

--D
Christoph Hellwig Feb. 29, 2024, 7:42 p.m. UTC | #6
On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> It turns out that xfs/122 also captures ioctl structure sizes, and those
> are /not/ captured by xfs_ondisk.h.  I think we should add those before
> we kill xfs/122.

Sure, I can look into that.
Zorro Lang March 1, 2024, 1:18 p.m. UTC | #7
On Thu, Feb 29, 2024 at 11:42:07AM -0800, Christoph Hellwig wrote:
> On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> > It turns out that xfs/122 also captures ioctl structure sizes, and those
> > are /not/ captured by xfs_ondisk.h.  I think we should add those before
> > we kill xfs/122.
> 
> Sure, I can look into that.

Hi Darrick,

Do you still want to have this patch?

Half of this patchset got RVB. As it's a random fix patchset, we can choose
merging those reviewed patches at first. Or you'd like to have them together
in next next release?

Thanks,
Zorro

> 
>
Darrick J. Wong March 1, 2024, 5:50 p.m. UTC | #8
On Fri, Mar 01, 2024 at 09:18:48PM +0800, Zorro Lang wrote:
> On Thu, Feb 29, 2024 at 11:42:07AM -0800, Christoph Hellwig wrote:
> > On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> > > It turns out that xfs/122 also captures ioctl structure sizes, and those
> > > are /not/ captured by xfs_ondisk.h.  I think we should add those before
> > > we kill xfs/122.
> > 
> > Sure, I can look into that.
> 
> Hi Darrick,
> 
> Do you still want to have this patch?
> 
> Half of this patchset got RVB. As it's a random fix patchset, we can choose
> merging those reviewed patches at first. Or you'd like to have them together
> in next next release?

I was about to resend the second to last patch.  If you decide to remove
xfs/122 then I'll drop this one.

--D

> Thanks,
> Zorro
> 
> > 
> > 
> 
>
Zorro Lang March 2, 2024, 4:55 a.m. UTC | #9
On Fri, Mar 01, 2024 at 09:50:20AM -0800, Darrick J. Wong wrote:
> On Fri, Mar 01, 2024 at 09:18:48PM +0800, Zorro Lang wrote:
> > On Thu, Feb 29, 2024 at 11:42:07AM -0800, Christoph Hellwig wrote:
> > > On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> > > > It turns out that xfs/122 also captures ioctl structure sizes, and those
> > > > are /not/ captured by xfs_ondisk.h.  I think we should add those before
> > > > we kill xfs/122.
> > > 
> > > Sure, I can look into that.
> > 
> > Hi Darrick,
> > 
> > Do you still want to have this patch?
> > 
> > Half of this patchset got RVB. As it's a random fix patchset, we can choose
> > merging those reviewed patches at first. Or you'd like to have them together
> > in next next release?
> 
> I was about to resend the second to last patch.  If you decide to remove
> xfs/122 then I'll drop this one.

xfs/122 is a xfs specific test case, it's more important for xfs list than me.
As it doesn't break the fstests testing, I respect the decision from xfs folks,
about keeping or removing it :)

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > 
> > > 
> > 
> > 
>
Darrick J. Wong March 7, 2024, 11:24 p.m. UTC | #10
On Sat, Mar 02, 2024 at 12:55:52PM +0800, Zorro Lang wrote:
> On Fri, Mar 01, 2024 at 09:50:20AM -0800, Darrick J. Wong wrote:
> > On Fri, Mar 01, 2024 at 09:18:48PM +0800, Zorro Lang wrote:
> > > On Thu, Feb 29, 2024 at 11:42:07AM -0800, Christoph Hellwig wrote:
> > > > On Thu, Feb 29, 2024 at 09:48:31AM -0800, Darrick J. Wong wrote:
> > > > > It turns out that xfs/122 also captures ioctl structure sizes, and those
> > > > > are /not/ captured by xfs_ondisk.h.  I think we should add those before
> > > > > we kill xfs/122.
> > > > 
> > > > Sure, I can look into that.
> > > 
> > > Hi Darrick,
> > > 
> > > Do you still want to have this patch?
> > > 
> > > Half of this patchset got RVB. As it's a random fix patchset, we can choose
> > > merging those reviewed patches at first. Or you'd like to have them together
> > > in next next release?
> > 
> > I was about to resend the second to last patch.  If you decide to remove
> > xfs/122 then I'll drop this one.
> 
> xfs/122 is a xfs specific test case, it's more important for xfs list than me.
> As it doesn't break the fstests testing, I respect the decision from xfs folks,
> about keeping or removing it :)

I think we shouldn't consider dropping this until there's an xfsprogs
release with xfs_ondisk.h in the build process.  Even then, my
preference would be to leave a mark in xfs_db somewhere so that we keep
running this test for old userspace (i.e. the mark isn't found).

--D

> Thanks,
> Zorro
> 
> > 
> > --D
> > 
> > > Thanks,
> > > Zorro
> > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> 
>
diff mbox series

Patch

diff --git a/tests/xfs/122 b/tests/xfs/122
index ba927c77c4..4e5ba1dfee 100755
--- a/tests/xfs/122
+++ b/tests/xfs/122
@@ -195,7 +195,7 @@  echo 'int main(int argc, char *argv[]) {' >>$cprog
 #
 cat /usr/include/xfs/xfs*.h | indent |\
 _attribute_filter |\
-grep -E '(} *xfs_.*_t|^struct xfs_[a-z0-9_]*$)' |\
+grep -E '(} *xfs_.*_t|^(union|struct) xfs_[a-z0-9_]*$)' |\
 grep -E -v -f $tmp.ignore |\
 sed -e 's/^.*}[[:space:]]*//g' -e 's/;.*$//g' -e 's/_t, /_t\n/g' |\
 sort | uniq |\
diff --git a/tests/xfs/122.out b/tests/xfs/122.out
index 067a0ec76b..a2b57cfb9b 100644
--- a/tests/xfs/122.out
+++ b/tests/xfs/122.out
@@ -124,6 +124,8 @@  sizeof(struct xfs_swap_extent) = 64
 sizeof(struct xfs_sxd_log_format) = 16
 sizeof(struct xfs_sxi_log_format) = 80
 sizeof(struct xfs_unmount_log_format) = 8
+sizeof(union xfs_rtword_raw) = 4
+sizeof(union xfs_suminfo_raw) = 4
 sizeof(xfs_agf_t) = 224
 sizeof(xfs_agfl_t) = 36
 sizeof(xfs_agi_t) = 344