[PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Eric Sandeen]

[*] Note: This needs to be merged as soon as possible as it's introducing an incompatible UAPI change...

STATX_ATTR_MOUNT_ROOT and STATX_ATTR_DAX got merged with the same value,
so one of them needs fixing. Move STATX_ATTR_DAX.

While we're in here, clarify the value-matching scheme for some of the
attributes, and explain why the value for DAX does not match.

Fixes: 80340fe3605c ("statx: add mount_root")
Fixes: 712b2698e4c0 ("fs/stat: Define DAX statx attribute")
Reported-by: David Howells <dhowells@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: David Howells <dhowells@redhat.com>
---
V2: Change flag value per Darrick Wong
    Tweak comment per Darrick Wong
    Add Fixes: tags & reported-by & RVB per dhowells

 include/uapi/linux/stat.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 82cc58fe9368..1500a0f58041 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -171,9 +171,12 @@ struct statx {
  * be of use to ordinary userspace programs such as GUIs or ls rather than
  * specialised tools.
  *
- * Note that the flags marked [I] correspond to generic FS_IOC_FLAGS
+ * Note that the flags marked [I] correspond to the FS_IOC_SETFLAGS flags
  * semantically.  Where possible, the numerical value is picked to correspond
- * also.
+ * also.  Note that the DAX attribute indicates that the file is in the CPU
+ * direct access state.  It does not correspond to the per-inode flag that
+ * some filesystems support.
+ *
  */
 #define STATX_ATTR_COMPRESSED		0x00000004 /* [I] File is compressed by the fs */
 #define STATX_ATTR_IMMUTABLE		0x00000010 /* [I] File is marked immutable */
@@ -183,7 +186,7 @@ struct statx {
 #define STATX_ATTR_AUTOMOUNT		0x00001000 /* Dir: Automount trigger */
 #define STATX_ATTR_MOUNT_ROOT		0x00002000 /* Root of a mount */
 #define STATX_ATTR_VERITY		0x00100000 /* [I] Verity protected file */
-#define STATX_ATTR_DAX			0x00002000 /* [I] File is DAX */
+#define STATX_ATTR_DAX			0x00200000 /* File is currently in DAX state */
 
 
 #endif /* _UAPI_LINUX_STAT_H */
-- 
2.17.0

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Darrick J. Wong]

On Tue, Dec 01, 2020 at 05:21:40PM -0600, Eric Sandeen wrote:
> [*] Note: This needs to be merged as soon as possible as it's introducing an incompatible UAPI change...
> 
> STATX_ATTR_MOUNT_ROOT and STATX_ATTR_DAX got merged with the same value,
> so one of them needs fixing. Move STATX_ATTR_DAX.
> 
> While we're in here, clarify the value-matching scheme for some of the
> attributes, and explain why the value for DAX does not match.
> 
> Fixes: 80340fe3605c ("statx: add mount_root")
> Fixes: 712b2698e4c0 ("fs/stat: Define DAX statx attribute")
> Reported-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: David Howells <dhowells@redhat.com>

Looks fine to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> V2: Change flag value per Darrick Wong
>     Tweak comment per Darrick Wong
>     Add Fixes: tags & reported-by & RVB per dhowells
> 
>  include/uapi/linux/stat.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> index 82cc58fe9368..1500a0f58041 100644
> --- a/include/uapi/linux/stat.h
> +++ b/include/uapi/linux/stat.h
> @@ -171,9 +171,12 @@ struct statx {
>   * be of use to ordinary userspace programs such as GUIs or ls rather than
>   * specialised tools.
>   *
> - * Note that the flags marked [I] correspond to generic FS_IOC_FLAGS
> + * Note that the flags marked [I] correspond to the FS_IOC_SETFLAGS flags
>   * semantically.  Where possible, the numerical value is picked to correspond
> - * also.
> + * also.  Note that the DAX attribute indicates that the file is in the CPU
> + * direct access state.  It does not correspond to the per-inode flag that
> + * some filesystems support.
> + *
>   */
>  #define STATX_ATTR_COMPRESSED		0x00000004 /* [I] File is compressed by the fs */
>  #define STATX_ATTR_IMMUTABLE		0x00000010 /* [I] File is marked immutable */
> @@ -183,7 +186,7 @@ struct statx {
>  #define STATX_ATTR_AUTOMOUNT		0x00001000 /* Dir: Automount trigger */
>  #define STATX_ATTR_MOUNT_ROOT		0x00002000 /* Root of a mount */
>  #define STATX_ATTR_VERITY		0x00100000 /* [I] Verity protected file */
> -#define STATX_ATTR_DAX			0x00002000 /* [I] File is DAX */
> +#define STATX_ATTR_DAX			0x00200000 /* File is currently in DAX state */
>  
>  
>  #endif /* _UAPI_LINUX_STAT_H */
> -- 
> 2.17.0
> 

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Christoph Hellwig]

Looks good for the urgent fix:

Reviewed-by: Christoph Hellwig <hch@lst.de>

We can keep debatting about stx_attributes_mask for a while once this
is sorted out :)

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Ira Weiny]

On Tue, Dec 01, 2020 at 05:21:40PM -0600, Eric Sandeen wrote:
> [*] Note: This needs to be merged as soon as possible as it's introducing an incompatible UAPI change...
> 
> STATX_ATTR_MOUNT_ROOT and STATX_ATTR_DAX got merged with the same value,
> so one of them needs fixing. Move STATX_ATTR_DAX.
> 
> While we're in here, clarify the value-matching scheme for some of the
> attributes, and explain why the value for DAX does not match.
> 
> Fixes: 80340fe3605c ("statx: add mount_root")
> Fixes: 712b2698e4c0 ("fs/stat: Define DAX statx attribute")
> Reported-by: David Howells <dhowells@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: David Howells <dhowells@redhat.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
> V2: Change flag value per Darrick Wong
>     Tweak comment per Darrick Wong
>     Add Fixes: tags & reported-by & RVB per dhowells
> 
>  include/uapi/linux/stat.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
> index 82cc58fe9368..1500a0f58041 100644
> --- a/include/uapi/linux/stat.h
> +++ b/include/uapi/linux/stat.h
> @@ -171,9 +171,12 @@ struct statx {
>   * be of use to ordinary userspace programs such as GUIs or ls rather than
>   * specialised tools.
>   *
> - * Note that the flags marked [I] correspond to generic FS_IOC_FLAGS
> + * Note that the flags marked [I] correspond to the FS_IOC_SETFLAGS flags
>   * semantically.  Where possible, the numerical value is picked to correspond
> - * also.
> + * also.  Note that the DAX attribute indicates that the file is in the CPU
> + * direct access state.  It does not correspond to the per-inode flag that
> + * some filesystems support.
> + *
>   */
>  #define STATX_ATTR_COMPRESSED		0x00000004 /* [I] File is compressed by the fs */
>  #define STATX_ATTR_IMMUTABLE		0x00000010 /* [I] File is marked immutable */
> @@ -183,7 +186,7 @@ struct statx {
>  #define STATX_ATTR_AUTOMOUNT		0x00001000 /* Dir: Automount trigger */
>  #define STATX_ATTR_MOUNT_ROOT		0x00002000 /* Root of a mount */
>  #define STATX_ATTR_VERITY		0x00100000 /* [I] Verity protected file */
> -#define STATX_ATTR_DAX			0x00002000 /* [I] File is DAX */
> +#define STATX_ATTR_DAX			0x00200000 /* File is currently in DAX state */
>  
>  
>  #endif /* _UAPI_LINUX_STAT_H */
> -- 
> 2.17.0
> 

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Miklos Szeredi]

On Wed, Dec 2, 2020 at 5:03 PM Ira Weiny <ira.weiny@intel.com> wrote:
>
> On Tue, Dec 01, 2020 at 05:21:40PM -0600, Eric Sandeen wrote:
> > [*] Note: This needs to be merged as soon as possible as it's introducing an incompatible UAPI change...
> >
> > STATX_ATTR_MOUNT_ROOT and STATX_ATTR_DAX got merged with the same value,
> > so one of them needs fixing. Move STATX_ATTR_DAX.
> >
> > While we're in here, clarify the value-matching scheme for some of the
> > attributes, and explain why the value for DAX does not match.
> >
> > Fixes: 80340fe3605c ("statx: add mount_root")
> > Fixes: 712b2698e4c0 ("fs/stat: Define DAX statx attribute")
> > Reported-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > Reviewed-by: David Howells <dhowells@redhat.com>
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Stable cc also?

Cc: <stable@vger.kernel.org> # 5.8

Thanks,
Miklos

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> Stable cc also?
> 
> Cc: <stable@vger.kernel.org> # 5.8

That seems to be unnecessary, provided there's a Fixes: tag.

David

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Miklos Szeredi]

On Wed, Dec 2, 2020 at 5:24 PM David Howells <dhowells@redhat.com> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > Stable cc also?
> >
> > Cc: <stable@vger.kernel.org> # 5.8
>
> That seems to be unnecessary, provided there's a Fixes: tag.

Is it?

Fixes: means it fixes a patch, Cc: stable means it needs to be
included in stable kernels.  The two are not necessarily the same.

Greg?

Thanks,
Miklos

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Greg Kroah-Hartman]

On Wed, Dec 02, 2020 at 06:41:43PM +0100, Miklos Szeredi wrote:
> On Wed, Dec 2, 2020 at 5:24 PM David Howells <dhowells@redhat.com> wrote:
> >
> > Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > > Stable cc also?
> > >
> > > Cc: <stable@vger.kernel.org> # 5.8
> >
> > That seems to be unnecessary, provided there's a Fixes: tag.
> 
> Is it?
> 
> Fixes: means it fixes a patch, Cc: stable means it needs to be
> included in stable kernels.  The two are not necessarily the same.
> 
> Greg?

You are correct.  cc: stable, as is documented in
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
ensures that the patch will get merged into the stable tree.

Fixes: is independent of it.  It's great to have for stable patches so
that I know how far back to backport patches.

We do scan all commits for Fixes: tags that do not have cc: stable, and
try to pick them up when we can and have the time to do so.  But it's
not guaranteed at all that this will happen.

I don't know why people keep getting confused about this, we don't
document the "Fixes: means it goes to stable" anywhere...

thanks,

greg k-h

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Dave Chinner]

On Wed, Dec 02, 2020 at 08:06:01PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Dec 02, 2020 at 06:41:43PM +0100, Miklos Szeredi wrote:
> > On Wed, Dec 2, 2020 at 5:24 PM David Howells <dhowells@redhat.com> wrote:
> > >
> > > Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > > Stable cc also?
> > > >
> > > > Cc: <stable@vger.kernel.org> # 5.8
> > >
> > > That seems to be unnecessary, provided there's a Fixes: tag.
> > 
> > Is it?
> > 
> > Fixes: means it fixes a patch, Cc: stable means it needs to be
> > included in stable kernels.  The two are not necessarily the same.
> > 
> > Greg?
> 
> You are correct.  cc: stable, as is documented in
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> ensures that the patch will get merged into the stable tree.
> 
> Fixes: is independent of it.  It's great to have for stable patches so
> that I know how far back to backport patches.
> 
> We do scan all commits for Fixes: tags that do not have cc: stable, and
> try to pick them up when we can and have the time to do so.  But it's
> not guaranteed at all that this will happen.
> 
> I don't know why people keep getting confused about this, we don't
> document the "Fixes: means it goes to stable" anywhere...

Except that is exactly what happens, sometimes within a day of two
of a patch with a Fixes tag hitting Linus' kernel. We have had a
serious XFS regression in the 5.9.9 stable kernel that should never
have happened as a result of exactly this "Fixes = automatically
swept immediately into stable kernels" behaviour. See here for
post-mortem analysis:

https://lore.kernel.org/linux-xfs/20201126071323.GF2842436@dread.disaster.area/T/#m26e14ebd28ad306025f4ebf37e2aae9a304345a5

This happened because these auotmated Fixes scans seem to occur
weekly during -rcX release periods, which means there really is *no
practical difference* between the way the stable process treats
Fixes tags and cc: stable.

Hence instead of developers having some control over "urgent, must
backport now" fixes versus fixes that still need the -rcX
stabilisation and integration testing to shake them out fully, the
regular scans result in everything with a fixes tag is treated as an
"urgent, must backport now" category of fix. It effectively
removes the stabilisation and integration testing process from
the changes stable kernel users are being exposed to...

That's not right. It gives upstream developers no margin for error,
and it exposes stable kernel users to bugs that the normal upstream
kernel stabilisation process prevents users from ever seeing in
released kernels. And it is exactly this behaviour that lead people
to understand that "fixes" and "cc: stable" essentially mean the
same thing from a stable kernel perspective.

It seems like this can all be avoided simply by scheduling the
automated fixes scans once the upstream kernel is released, not
while it is still being stabilised by -rc releases. That way stable
kernels get better tested fixes, they still get the same quantity of
fixes, and upstream developers have some margin to detect and
correct regressions in fixes before they get propagated to users.

It also creates a clear demarcation between fixes and cc: stable for
maintainers and developers: only patches with a cc: stable will be
backported immediately to stable. Developers know what patches need
urgent backports and, unlike developers, the automated fixes scan
does not have the subject matter expertise or background to make
that judgement....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Greg Kroah-Hartman]

On Thu, Dec 03, 2020 at 07:40:45AM +1100, Dave Chinner wrote:
> On Wed, Dec 02, 2020 at 08:06:01PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Dec 02, 2020 at 06:41:43PM +0100, Miklos Szeredi wrote:
> > > On Wed, Dec 2, 2020 at 5:24 PM David Howells <dhowells@redhat.com> wrote:
> > > >
> > > > Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > > Stable cc also?
> > > > >
> > > > > Cc: <stable@vger.kernel.org> # 5.8
> > > >
> > > > That seems to be unnecessary, provided there's a Fixes: tag.
> > > 
> > > Is it?
> > > 
> > > Fixes: means it fixes a patch, Cc: stable means it needs to be
> > > included in stable kernels.  The two are not necessarily the same.
> > > 
> > > Greg?
> > 
> > You are correct.  cc: stable, as is documented in
> >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > ensures that the patch will get merged into the stable tree.
> > 
> > Fixes: is independent of it.  It's great to have for stable patches so
> > that I know how far back to backport patches.
> > 
> > We do scan all commits for Fixes: tags that do not have cc: stable, and
> > try to pick them up when we can and have the time to do so.  But it's
> > not guaranteed at all that this will happen.
> > 
> > I don't know why people keep getting confused about this, we don't
> > document the "Fixes: means it goes to stable" anywhere...
> 
> Except that is exactly what happens, sometimes within a day of two
> of a patch with a Fixes tag hitting Linus' kernel. We have had a
> serious XFS regression in the 5.9.9 stable kernel that should never
> have happened as a result of exactly this "Fixes = automatically
> swept immediately into stable kernels" behaviour. See here for
> post-mortem analysis:
> 
> https://lore.kernel.org/linux-xfs/20201126071323.GF2842436@dread.disaster.area/T/#m26e14ebd28ad306025f4ebf37e2aae9a304345a5
> 
> This happened because these auotmated Fixes scans seem to occur
> weekly during -rcX release periods, which means there really is *no
> practical difference* between the way the stable process treats
> Fixes tags and cc: stable.

Sometimes, yes, that is true.  But as it went into Linus's tree at the
same time, we just ended up with "bug compatible" trees :)

Not a big deal overall, happens every few releases, we fix it up and
move on.  The benifits in doing all of this _FAR_ outweigh the very
infrequent times that kernel developers get something wrong.

As always, if you do NOT want your subsystem to have fixes: tags picked
up automatically by us for stable trees, just email us and let us know
to not do that and we gladly will.

> It seems like this can all be avoided simply by scheduling the
> automated fixes scans once the upstream kernel is released, not
> while it is still being stabilised by -rc releases. That way stable
> kernels get better tested fixes, they still get the same quantity of
> fixes, and upstream developers have some margin to detect and
> correct regressions in fixes before they get propagated to users.

So the "magic" -final release from Linus would cause this to happen?
That means that the world would go for 3 months without some known fixes
being applied to the tree?  That's not acceptable to me, as I started
doing this because it was needed to be done, not just because I wanted
to do more work...

> It also creates a clear demarcation between fixes and cc: stable for
> maintainers and developers: only patches with a cc: stable will be
> backported immediately to stable. Developers know what patches need
> urgent backports and, unlike developers, the automated fixes scan
> does not have the subject matter expertise or background to make
> that judgement....

Some subsystems do not have such clear demarcation at all.  Heck, some
subsystems don't even add a cc: stable to known major fixes.  And that's
ok, the goal of the stable kernel work is to NOT impose additional work
on developers or maintainers if they don't want to do that work.

thanks,

greg k-h

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Dave Chinner]

On Wed, Dec 02, 2020 at 10:04:17PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Dec 03, 2020 at 07:40:45AM +1100, Dave Chinner wrote:
> > On Wed, Dec 02, 2020 at 08:06:01PM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Dec 02, 2020 at 06:41:43PM +0100, Miklos Szeredi wrote:
> > > > On Wed, Dec 2, 2020 at 5:24 PM David Howells <dhowells@redhat.com> wrote:
> > > > >
> > > > > Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >
> > > > > > Stable cc also?
> > > > > >
> > > > > > Cc: <stable@vger.kernel.org> # 5.8
> > > > >
> > > > > That seems to be unnecessary, provided there's a Fixes: tag.
> > > > 
> > > > Is it?
> > > > 
> > > > Fixes: means it fixes a patch, Cc: stable means it needs to be
> > > > included in stable kernels.  The two are not necessarily the same.
> > > > 
> > > > Greg?
> > > 
> > > You are correct.  cc: stable, as is documented in
> > >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > > ensures that the patch will get merged into the stable tree.
> > > 
> > > Fixes: is independent of it.  It's great to have for stable patches so
> > > that I know how far back to backport patches.
> > > 
> > > We do scan all commits for Fixes: tags that do not have cc: stable, and
> > > try to pick them up when we can and have the time to do so.  But it's
> > > not guaranteed at all that this will happen.
> > > 
> > > I don't know why people keep getting confused about this, we don't
> > > document the "Fixes: means it goes to stable" anywhere...
> > 
> > Except that is exactly what happens, sometimes within a day of two
> > of a patch with a Fixes tag hitting Linus' kernel. We have had a
> > serious XFS regression in the 5.9.9 stable kernel that should never
> > have happened as a result of exactly this "Fixes = automatically
> > swept immediately into stable kernels" behaviour. See here for
> > post-mortem analysis:
> > 
> > https://lore.kernel.org/linux-xfs/20201126071323.GF2842436@dread.disaster.area/T/#m26e14ebd28ad306025f4ebf37e2aae9a304345a5
> > 
> > This happened because these auotmated Fixes scans seem to occur
> > weekly during -rcX release periods, which means there really is *no
> > practical difference* between the way the stable process treats
> > Fixes tags and cc: stable.
> 
> Sometimes, yes, that is true.  But as it went into Linus's tree at the
> same time, we just ended up with "bug compatible" trees :)
> 
> Not a big deal overall, happens every few releases, we fix it up and
> move on.  The benifits in doing all of this _FAR_ outweigh the very
> infrequent times that kernel developers get something wrong.

I'm not debating that users benefit from backports. I'm talking
about managing risk profiles and how to prevent an entirely
preventable stable kernel regression from happening again.

Talking about risk profiles, the issue here is that the regression
that slipped through to the stable kernels had a -catastrophic- risk
profile. That's exactly the sort of things that the stable kernel is
supposed to avoid exposing users to, and that raises the importance
and priority of ensuring that *never happens again*.

And the cause of this regression slipping through to stable kernel
users? It was a result of the automated "fixes" scan done by the
stable process that results in "fixes" meaning the same thing as
"cc: stable"....

> As always, if you do NOT want your subsystem to have fixes: tags picked
> up automatically by us for stable trees, just email us and let us know
> to not do that and we gladly will.

No, that is not an acceptible solution for anyone. The stable
maintainers need to stop suggesting this as a solution to any
criticism that is raised against the stable process. You may as well
just say "shut up, go away, we don't care what you want".

> > It seems like this can all be avoided simply by scheduling the
> > automated fixes scans once the upstream kernel is released, not
> > while it is still being stabilised by -rc releases. That way stable
> > kernels get better tested fixes, they still get the same quantity of
> > fixes, and upstream developers have some margin to detect and
> > correct regressions in fixes before they get propagated to users.
> 
> So the "magic" -final release from Linus would cause this to happen?
> That means that the world would go for 3 months without some known fixes
> being applied to the tree?  That's not acceptable to me, as I started
> doing this because it was needed to be done, not just because I wanted
> to do more work...

I'm not suggesting that all fixes across the entire kernel get held
until release. That's just taking things to extremes for no valid
reason as the risk profiles of most subsystems don't justify needing
a margin of error that large. I'm asking that specific subsystems
with catastrophic failure risk profiles be allowed to opt
out of the "just merged" fixes scans and instead have them replaced
by a less frequent scan.

Perhaps we don't even need to wait for the full release. Maybe just
increasing the fixes scanning window for those subsystems to pick up
changes in -rc(X-2) so that the commits have been exposed to testing
for a couple of weeks before being considered a stable backport
candidate. 

That mitigates the immediate risk concern as it gives developers
time to catch and fix regressions before stable backports are done.
Such a 2 week delay would have avoided exposing stable kernel users
to dangerous regression that should never have been released outside
developer and test machines exercising the upstream -rcX tree.

> > It also creates a clear demarcation between fixes and cc: stable for
> > maintainers and developers: only patches with a cc: stable will be
> > backported immediately to stable. Developers know what patches need
> > urgent backports and, unlike developers, the automated fixes scan
> > does not have the subject matter expertise or background to make
> > that judgement....
> 
> Some subsystems do not have such clear demarcation at all. Heck, some
> subsystems don't even add a cc: stable to known major fixes.  And that's
> ok, the goal of the stable kernel work is to NOT impose additional work
> on developers or maintainers if they don't want to do that work.

Engineering is as much about improving processes as it is about
improving the thing that is being built.  I'm not asking you to stop
backporting fixes or stop improving the stable kernels. All I'm
asking for is to increase the latency of backports for some
subsystems because a margin of error is needed to minimise the risk
profile stable users are exposed to. IOWs, I'm asking for a *minor
tweak* to the existing process, not asking you to start all over
again.

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

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Amir Goldstein]

> > It seems like this can all be avoided simply by scheduling the
> > automated fixes scans once the upstream kernel is released, not
> > while it is still being stabilised by -rc releases. That way stable
> > kernels get better tested fixes, they still get the same quantity of
> > fixes, and upstream developers have some margin to detect and
> > correct regressions in fixes before they get propagated to users.
>
> So the "magic" -final release from Linus would cause this to happen?
> That means that the world would go for 3 months without some known fixes
> being applied to the tree?  That's not acceptable to me, as I started
> doing this because it was needed to be done, not just because I wanted
> to do more work...
>

Nobody was trying to undermine the need for expediting important fixes
into stable kernels. Quite the contrary.

> > It also creates a clear demarcation between fixes and cc: stable for
> > maintainers and developers: only patches with a cc: stable will be
> > backported immediately to stable. Developers know what patches need
> > urgent backports and, unlike developers, the automated fixes scan
> > does not have the subject matter expertise or background to make
> > that judgement....
>
> Some subsystems do not have such clear demarcation at all.  Heck, some
> subsystems don't even add a cc: stable to known major fixes.  And that's
> ok, the goal of the stable kernel work is to NOT impose additional work
> on developers or maintainers if they don't want to do that work.
>

Greg,

Please acknowledge that there is something to improve.
Saying that some subsystems maintainers don't care is not a great
argument for subsystem maintainers that do care and try to improve the process.

I am speaking here both as a maintainer of a downstream stable kernel,
who cares specifically about xfs fixes and as an upstream developer who
"contributes" patches to stable kernels. And I am not a passive contributor
to stable kernels. I try to take good care of overlayfs and fsnotify patches
being properly routed to stable kernels, as well as prepping the patches
for backport-ability during review and occasional backporting.
I also try to help with auditing the AUTOSEL patch selection of xfs.

The process can improve. This is an indisputable fact, because as contributors
we want to improve the quality of the stable kernels but missing the
tools to do so.

As a downstream user of stable kernels I learned to wait out a few .y releases
after xfs fixes have flowed in. This is possible because xfs stable
fixes are not
flowing that often.
Do you see what happened? You did not make the problem go away, but pushed
it down to your downstream users.
I would not have complained unless I thought that we could do better.

Here is a recent example, where during patch review, I requested NOT to include
any stable backport triggers [1]:
"...We should consider sending this to stable, but maybe let's merge
first and let it
 run in master for a while before because it is not a clear and
immediate danger..."

This is just one patch and I put a mental trigger to myself to stop it
during stable
patch review if it gets selected, but you can see how this solution
does not scale.

As a developer and as a reviewer, I wish (as Dave implied) that I had a way to
communicate to AUTOSEL that auto backport of this patch has more risk than
the risk of not backporting. I could also use a way to communicate
that this patch
(although may fix a bug) should be "treated as a feature", meaning that it needs
a full release cycle to stabilize should not see the light of day
before the upstream
.0 release. Some fixes are just like that.

The question is how to annotate these changes.
Thinking out loud:
    Cc: stable@vger.kernel.org#v5.9<<v5.10
    Cc: stable@vger.kernel.org#v5.9<<v5.10-rc5

For patches that need to soak a few cycles in master or need to linger in
master until the .0 release.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAOQ4uxiUTsXEdQsE275qxTh61tZOB+-wqCp6gaNLkOw5ueUJgw@mail.gmail.com/

Re: [PATCH V2] uapi: fix statx attribute value overlap for DAX & MOUNT_ROOT

[Theodore Y. Ts'o]

On Thu, Dec 03, 2020 at 08:18:23AM +0200, Amir Goldstein wrote:
> Here is a recent example, where during patch review, I requested NOT to include
> any stable backport triggers [1]:
> "...We should consider sending this to stable, but maybe let's merge
> first and let it
>  run in master for a while before because it is not a clear and
> immediate danger..."
>
> As a developer and as a reviewer, I wish (as Dave implied) that I had a way to
> communicate to AUTOSEL that auto backport of this patch has more risk than
> the risk of not backporting.

My suggestion is that we could put something in the MAINTAINERS file
which indicates what the preferred delay time should be for (a)
patches explicitly cc'ed to stable, and (b) preferred time should be
for patches which are AUTOSEL'ed for stable for that subsystem.  That
time might be either in days/weeks, or "after N -rc releases", "after
the next full release", or, "never" (which would be a way for a
subsystem to opt out of the AUTOSEL process).

It should also be possible specify the delay in the trailer, e.g.:

Stable-Defer: <delayspec>
Auto-Stable-Defer: <delayspec>

The advantage of specifying the delay relative to when they show up in
Linus's tree helps deal with the case where the submaintainer might
not be sure when their patches will get pushed to Linus by the
maintainer.

Cheers,

						- Ted