[PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

It is vitally important that we preserve the state of the NREXT64 inode
flag when we're changing the other flags2 fields.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_ioctl.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5a364a7d58fd..0d67ff8a8961 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1096,7 +1096,8 @@ xfs_flags2diflags2(
 {
 	uint64_t		di_flags2 =
 		(ip->i_diflags2 & (XFS_DIFLAG2_REFLINK |
-				   XFS_DIFLAG2_BIGTIME));
+				   XFS_DIFLAG2_BIGTIME |
+				   XFS_DIFLAG2_NREXT64));
 
 	if (xflags & FS_XFLAG_DAX)
 		di_flags2 |= XFS_DIFLAG2_DAX;

Re: [PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Dave Chinner]

On Sun, Jun 05, 2022 at 09:35:43AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> It is vitally important that we preserve the state of the NREXT64 inode
> flag when we're changing the other flags2 fields.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_ioctl.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Fixes tag?

> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5a364a7d58fd..0d67ff8a8961 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1096,7 +1096,8 @@ xfs_flags2diflags2(
>  {
>  	uint64_t		di_flags2 =
>  		(ip->i_diflags2 & (XFS_DIFLAG2_REFLINK |
> -				   XFS_DIFLAG2_BIGTIME));
> +				   XFS_DIFLAG2_BIGTIME |
> +				   XFS_DIFLAG2_NREXT64));
>  
>  	if (xflags & FS_XFLAG_DAX)
>  		di_flags2 |= XFS_DIFLAG2_DAX;

Otherwise looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Darrick J. Wong]

On Mon, Jun 06, 2022 at 08:29:40AM +1000, Dave Chinner wrote:
> On Sun, Jun 05, 2022 at 09:35:43AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > It is vitally important that we preserve the state of the NREXT64 inode
> > flag when we're changing the other flags2 fields.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_ioctl.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Fixes tag?

Does this really need one?  NREXT64 is still experimental, none of this
code is in any released kernel, and I'm not even sure what would be a
good target -- the patch that introduced XFS_DIFLAG_NREXT64?

> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 5a364a7d58fd..0d67ff8a8961 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1096,7 +1096,8 @@ xfs_flags2diflags2(
> >  {
> >  	uint64_t		di_flags2 =
> >  		(ip->i_diflags2 & (XFS_DIFLAG2_REFLINK |
> > -				   XFS_DIFLAG2_BIGTIME));
> > +				   XFS_DIFLAG2_BIGTIME |
> > +				   XFS_DIFLAG2_NREXT64));
> >  
> >  	if (xflags & FS_XFLAG_DAX)
> >  		di_flags2 |= XFS_DIFLAG2_DAX;
> 
> Otherwise looks good.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thx. :)

--D

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Dave Chinner]

On Sun, Jun 05, 2022 at 05:02:33PM -0700, Darrick J. Wong wrote:
> On Mon, Jun 06, 2022 at 08:29:40AM +1000, Dave Chinner wrote:
> > On Sun, Jun 05, 2022 at 09:35:43AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > It is vitally important that we preserve the state of the NREXT64 inode
> > > flag when we're changing the other flags2 fields.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/xfs_ioctl.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > Fixes tag?
> 
> Does this really need one?  NREXT64 is still experimental, none of this
> code is in any released kernel,

In my opinion, absolutely not.

But after just spending a large part of the last cycle fielding
unjustified complaint after complaint that we didn't get everything
exactly perfect for every possible unforseen future uses for commit
messages and cover letters....

> and I'm not even sure what would be a
> good target -- the patch that introduced XFS_DIFLAG_NREXT64?

9b7d16e34bbe xfs: Introduce XFS_DIFLAG2_NREXT64 and associated helpers

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Amir Goldstein]

On Mon, Jun 6, 2022 at 8:24 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Mon, Jun 06, 2022 at 08:29:40AM +1000, Dave Chinner wrote:
> > On Sun, Jun 05, 2022 at 09:35:43AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > It is vitally important that we preserve the state of the NREXT64 inode
> > > flag when we're changing the other flags2 fields.
> > >
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/xfs_ioctl.c |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Fixes tag?

Thank you Dave!

>
> Does this really need one?

I say why not.

I am not looking for a fight. Really, it's up to xfs maintainers how to manage
experimental features. That is completely outside of scope for LTS.
I only want to explain my POV as a developer.

You know my interest is in backporting fixes for LTS, so this one won't be
relevant anyway, but if I were you, I would send this patch to stable 5.18.y
to *reduce* burden on myself -

The mental burden of having to carry the doubt of whether a certain
reported bug could have been involved with user booting into 5.18.y
and back.

When you think about it, it kind of makes sense to have the latest .y
in your grub menu when you are running upstream...
Users do that - heck, user do anything you don't want them to do,
even if eventually you can tell the users they did something that is
not expected to work, you had already invested the time in triage.

Sure, there is always the possibility that someone in the future of 5.19.y
will boot into 5.18.0, but that is a far less likely possibility.

For this reason, when I write new features I really try to treat the .y
release as the true release cycle of that feature rather than the .0,
regardless of LTS.
If I were the developer of the feature, I would have wanted to see
this fix applied to 5.18.y.

Thanks,
Amir.

Re: [PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Chandan Babu R]

On Sun, Jun 05, 2022 at 09:35:43 AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> It is vitally important that we preserve the state of the NREXT64 inode
> flag when we're changing the other flags2 fields.
>

Thanks for spotting and fixing this bug.

Reviewed-by: Chandan Babu R <chandan.babu@oracle.com>

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_ioctl.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5a364a7d58fd..0d67ff8a8961 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1096,7 +1096,8 @@ xfs_flags2diflags2(
>  {
>  	uint64_t		di_flags2 =
>  		(ip->i_diflags2 & (XFS_DIFLAG2_REFLINK |
> -				   XFS_DIFLAG2_BIGTIME));
> +				   XFS_DIFLAG2_BIGTIME |
> +				   XFS_DIFLAG2_NREXT64));
>  
>  	if (xflags & FS_XFLAG_DAX)
>  		di_flags2 |= XFS_DIFLAG2_DAX;


-- 
chandan

Re: [PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Darrick J. Wong]

On Mon, Jun 06, 2022 at 10:22:03AM +0300, Amir Goldstein wrote:
> On Mon, Jun 6, 2022 at 8:24 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Mon, Jun 06, 2022 at 08:29:40AM +1000, Dave Chinner wrote:
> > > On Sun, Jun 05, 2022 at 09:35:43AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > >
> > > > It is vitally important that we preserve the state of the NREXT64 inode
> > > > flag when we're changing the other flags2 fields.
> > > >
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  fs/xfs/xfs_ioctl.c |    3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > Fixes tag?
> 
> Thank you Dave!
> 
> >
> > Does this really need one?
> 
> I say why not.

Every one of these asks adds friction for patch authors.  For code that
has already shipped in a Linus release it's a reasonable ask, but...

> I am not looking for a fight. Really, it's up to xfs maintainers how to manage
> experimental features. That is completely outside of scope for LTS.
> I only want to explain my POV as a developer.
> 
> You know my interest is in backporting fixes for LTS, so this one won't be
> relevant anyway, but if I were you, I would send this patch to stable 5.18.y
> to *reduce* burden on myself -

...WHY?

This is a fix for a new ondisk feature that landed in 5.19-rc1.  The
feature is EXPERIMENTAL, which means that it **should not** be
backported to 5.18, 5.15, or any other LTS kernel.  New features do NOT
fit the criteria for LTS backports.

That's why I didn't bother attaching a fixes tag!

> The mental burden of having to carry the doubt of whether a certain
> reported bug could have been involved with user booting into 5.18.y
> and back.
> 
> When you think about it, it kind of makes sense to have the latest .y
> in your grub menu when you are running upstream...
> Users do that - heck, user do anything you don't want them to do,
> even if eventually you can tell the users they did something that is
> not expected to work, you had already invested the time in triage.
> 
> Sure, there is always the possibility that someone in the future of 5.19.y
> will boot into 5.18.0, but that is a far less likely possibility.
> 
> For this reason, when I write new features I really try to treat the .y
> release as the true release cycle of that feature rather than the .0,
> regardless of LTS.
> If I were the developer of the feature, I would have wanted to see
> this fix applied to 5.18.y.

This fix **WILL NOT APPLY** to 5.18!

--D

> Thanks,
> Amir.

Re: [PATCH] xfs: preserve DIFLAG2_NREXT64 when setting other inode attributes

[Amir Goldstein]

On Mon, Jun 6, 2022 at 8:12 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Mon, Jun 06, 2022 at 10:22:03AM +0300, Amir Goldstein wrote:
> > On Mon, Jun 6, 2022 at 8:24 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > On Mon, Jun 06, 2022 at 08:29:40AM +1000, Dave Chinner wrote:
> > > > On Sun, Jun 05, 2022 at 09:35:43AM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > >
> > > > > It is vitally important that we preserve the state of the NREXT64 inode
> > > > > flag when we're changing the other flags2 fields.
> > > > >
> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > > >  fs/xfs/xfs_ioctl.c |    3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > Fixes tag?
> >
> > Thank you Dave!
> >
> > >
> > > Does this really need one?
> >
> > I say why not.
>
> Every one of these asks adds friction for patch authors.  For code that
> has already shipped in a Linus release it's a reasonable ask, but...
>
> > I am not looking for a fight. Really, it's up to xfs maintainers how to manage
> > experimental features. That is completely outside of scope for LTS.
> > I only want to explain my POV as a developer.
> >
> > You know my interest is in backporting fixes for LTS, so this one won't be
> > relevant anyway, but if I were you, I would send this patch to stable 5.18.y
> > to *reduce* burden on myself -
>
> ...WHY?
>
> This is a fix for a new ondisk feature that landed in 5.19-rc1.  The
> feature is EXPERIMENTAL, which means that it **should not** be
> backported to 5.18, 5.15, or any other LTS kernel.  New features do NOT
> fit the criteria for LTS backports.
>
> That's why I didn't bother attaching a fixes tag!
>
> > The mental burden of having to carry the doubt of whether a certain
> > reported bug could have been involved with user booting into 5.18.y
> > and back.
> >
> > When you think about it, it kind of makes sense to have the latest .y
> > in your grub menu when you are running upstream...
> > Users do that - heck, user do anything you don't want them to do,
> > even if eventually you can tell the users they did something that is
> > not expected to work, you had already invested the time in triage.
> >
> > Sure, there is always the possibility that someone in the future of 5.19.y
> > will boot into 5.18.0, but that is a far less likely possibility.
> >
> > For this reason, when I write new features I really try to treat the .y
> > release as the true release cycle of that feature rather than the .0,
> > regardless of LTS.
> > If I were the developer of the feature, I would have wanted to see
> > this fix applied to 5.18.y.
>
> This fix **WILL NOT APPLY** to 5.18!
>

I stand corrected.
Sorry for the noise.

Amir.