| Message ID | Z-QYjLJk8_ttf-kW@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfs: add dummy definition for nfsd_file | expand |
On 3/26/2025 8:09 AM, Mike Snitzer wrote: > Add dummy definition for nfsd_file in both nfslocalio.c and localio.c > so older gcc (e.g. EL8's 8.5.0) can be used. Older gcc causes RCU > code (rcu_dereference and rcu_access_pointer) to dereference what > should just be an opaque pointer with its use of typeof. > > So without the dummy definition compiling with older gcc fails. > > Link: https://lore.kernel.org/all/Zsyhco1OrOI_uSbd@kernel.org/ > Fixes: 55a9742d02eff ("nfs: cache all open LOCALIO nfsd_file(s) in client") I saw this issue using crosstools/gcc-13.3.0-nolibc and this patch fixes it. Tested-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com> > Signed-off-by: Mike Snitzer <snitzer@hammerspace.com> note this doesn't match the From: address
On Wednesday 26 March 2025 08:33:32 Jeff Johnson wrote: > On 3/26/2025 8:09 AM, Mike Snitzer wrote: > > Add dummy definition for nfsd_file in both nfslocalio.c and localio.c > > so older gcc (e.g. EL8's 8.5.0) can be used. Older gcc causes RCU > > code (rcu_dereference and rcu_access_pointer) to dereference what > > should just be an opaque pointer with its use of typeof. > > > > So without the dummy definition compiling with older gcc fails. > > > > Link: https://lore.kernel.org/all/Zsyhco1OrOI_uSbd@kernel.org/ > > Fixes: 55a9742d02eff ("nfs: cache all open LOCALIO nfsd_file(s) in client") As this change is fixing compile error, should not be there also cc: stable line? > > I saw this issue using crosstools/gcc-13.3.0-nolibc and this patch fixes it. So maybe the commit message can be adjusted, so it does not say only "older gcc"? > Tested-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com> I have tested this change and it fixed compilation for me too. So: Tested-by: Pali Rohár <pali@kernel.org> > > > Signed-off-by: Mike Snitzer <snitzer@hammerspace.com> > > note this doesn't match the From: address > >
On Wed, Mar 26, 2025 at 09:59:19PM +0100, Pali Rohár wrote: > On Wednesday 26 March 2025 08:33:32 Jeff Johnson wrote: > > On 3/26/2025 8:09 AM, Mike Snitzer wrote: > > > Add dummy definition for nfsd_file in both nfslocalio.c and localio.c > > > so older gcc (e.g. EL8's 8.5.0) can be used. Older gcc causes RCU > > > code (rcu_dereference and rcu_access_pointer) to dereference what > > > should just be an opaque pointer with its use of typeof. > > > > > > So without the dummy definition compiling with older gcc fails. > > > > > > Link: https://lore.kernel.org/all/Zsyhco1OrOI_uSbd@kernel.org/ > > > Fixes: 55a9742d02eff ("nfs: cache all open LOCALIO nfsd_file(s) in client") > > As this change is fixing compile error, should not be there also cc: stable line? Any commit with a Fixes: tag will automatically be picked up by stable@ once it is merged. An explicit cc: sttable@ would be redundant. > > > > I saw this issue using crosstools/gcc-13.3.0-nolibc and this patch fixes it. > > So maybe the commit message can be adjusted, so it does not say only > "older gcc"? I don't see the need to list all compilers, I documented the compiler that motivated my fix. Fact that it applicable for crosstools/gcc-13.3.0-nolibc (which I don't have context for what it is.. but if this commit is needed for it then it is a suspect "new" compiler). > > Tested-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com> > > I have tested this change and it fixed compilation for me too. So: > > Tested-by: Pali Rohár <pali@kernel.org> > > > > > > Signed-off-by: Mike Snitzer <snitzer@hammerspace.com> > > > > note this doesn't match the From: address AFAIK there is no requirement that an S-o-B tag must match the email header's From. Mike
On Wednesday 26 March 2025 22:00:02 Mike Snitzer wrote: > On Wed, Mar 26, 2025 at 09:59:19PM +0100, Pali Rohár wrote: > > On Wednesday 26 March 2025 08:33:32 Jeff Johnson wrote: > > > On 3/26/2025 8:09 AM, Mike Snitzer wrote: > > > > Add dummy definition for nfsd_file in both nfslocalio.c and localio.c > > > > so older gcc (e.g. EL8's 8.5.0) can be used. Older gcc causes RCU > > > > code (rcu_dereference and rcu_access_pointer) to dereference what > > > > should just be an opaque pointer with its use of typeof. > > > > > > > > So without the dummy definition compiling with older gcc fails. > > > > > > > > Link: https://lore.kernel.org/all/Zsyhco1OrOI_uSbd@kernel.org/ > > > > Fixes: 55a9742d02eff ("nfs: cache all open LOCALIO nfsd_file(s) in client") > > > > As this change is fixing compile error, should not be there also cc: stable line? > > Any commit with a Fixes: tag will automatically be picked up by > stable@ once it is merged. An explicit cc: sttable@ would be > redundant. From my experience, when the backporting of commit with both Fixes:+Cc: fails then the committer of the patch and also other is informed by robot email. If there is only Fixes: without Cc: then people are not informed. So I used the Fixes:+Cc: for such build issues or important issues to ensure that the change is really backported. > > > > > > I saw this issue using crosstools/gcc-13.3.0-nolibc and this patch fixes it. > > > > So maybe the commit message can be adjusted, so it does not say only > > "older gcc"? > > I don't see the need to list all compilers, I documented the compiler > that motivated my fix. Fact that it applicable for > crosstools/gcc-13.3.0-nolibc (which I don't have context for what it > is.. but if this commit is needed for it then it is a suspect "new" > compiler). My impression was that the commit is fixing just the compilation with old gcc versions. But it seems that also new are affected. That is why I suggested to adjust it, so it would be clear that it applies for new gcc versions too. > > > Tested-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com> > > > > I have tested this change and it fixed compilation for me too. So: > > > > Tested-by: Pali Rohár <pali@kernel.org> > > > > > > > > > Signed-off-by: Mike Snitzer <snitzer@hammerspace.com> > > > > > > note this doesn't match the From: address > > AFAIK there is no requirement that an S-o-B tag must match the email > header's From. > > Mike
On Thu, Mar 27, 2025 at 09:28:33AM +0100, Pali Rohár wrote: > On Wednesday 26 March 2025 22:00:02 Mike Snitzer wrote: > > On Wed, Mar 26, 2025 at 09:59:19PM +0100, Pali Rohár wrote: > > > On Wednesday 26 March 2025 08:33:32 Jeff Johnson wrote: > > > > On 3/26/2025 8:09 AM, Mike Snitzer wrote: > > > > > Add dummy definition for nfsd_file in both nfslocalio.c and localio.c > > > > > so older gcc (e.g. EL8's 8.5.0) can be used. Older gcc causes RCU > > > > > code (rcu_dereference and rcu_access_pointer) to dereference what > > > > > should just be an opaque pointer with its use of typeof. > > > > > > > > > > So without the dummy definition compiling with older gcc fails. > > > > > > > > > > Link: https://lore.kernel.org/all/Zsyhco1OrOI_uSbd@kernel.org/ > > > > > Fixes: 55a9742d02eff ("nfs: cache all open LOCALIO nfsd_file(s) in client") > > > > > > As this change is fixing compile error, should not be there also cc: stable line? > > > > Any commit with a Fixes: tag will automatically be picked up by > > stable@ once it is merged. An explicit cc: sttable@ would be > > redundant. > > From my experience, when the backporting of commit with both Fixes:+Cc: > fails then the committer of the patch and also other is informed by > robot email. If there is only Fixes: without Cc: then people are not > informed. > > So I used the Fixes:+Cc: for such build issues or important issues to > ensure that the change is really backported. If a Fixes commit isn't able to be easily backported then mail is sent giving notice to the author. That has always been sufficient for anything I have authored. > > > > > > > > I saw this issue using crosstools/gcc-13.3.0-nolibc and this patch fixes it. > > > > > > So maybe the commit message can be adjusted, so it does not say only > > > "older gcc"? > > > > I don't see the need to list all compilers, I documented the compiler > > that motivated my fix. Fact that it applicable for > > crosstools/gcc-13.3.0-nolibc (which I don't have context for what it > > is.. but if this commit is needed for it then it is a suspect "new" > > compiler). > > My impression was that the commit is fixing just the compilation with > old gcc versions. But it seems that also new are affected. That is why I > suggested to adjust it, so it would be clear that it applies for new gcc > versions too. Really not sure why you're being so pedantic. I submitted what I developed as a fix for my needs with an old gcc (from EL8) and so I submitted what I had. We don't need to impose every compiler be covered in a commit header (especially when the new case occurred 4 months later). crosstools/gcc-13.3.0-nolibc is not something I'm going to hunt down and pick apart to prove that it is using old compiler technology, but I'm inclined to think it is. Mike
diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c index 5c21caeae075..830078e5866b 100644 --- a/fs/nfs/localio.c +++ b/fs/nfs/localio.c @@ -46,6 +46,14 @@ struct nfs_local_fsync_ctx { struct completion *done; }; +/* + * nfsd_file structure is purposely kept opaque to NFS client. + * This is a dummy definition to make RCU compilation happy. + */ +struct nfsd_file { + int undefined__; +}; + static bool localio_enabled __read_mostly = true; module_param(localio_enabled, bool, 0644); diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c index 6a0bdea6d644..f3274a70ce5e 100644 --- a/fs/nfs_common/nfslocalio.c +++ b/fs/nfs_common/nfslocalio.c @@ -271,6 +271,14 @@ struct nfsd_file *nfs_open_local_fh(nfs_uuid_t *uuid, } EXPORT_SYMBOL_GPL(nfs_open_local_fh); +/* + * nfsd_file structure is purposely kept opaque to NFS client. + * This is a dummy definition to make RCU compilation happy. + */ +struct nfsd_file { + int undefined__; +}; + void nfs_close_local_fh(struct nfs_file_localio *nfl) { struct nfsd_file *ro_nf = NULL;
Add dummy definition for nfsd_file in both nfslocalio.c and localio.c so older gcc (e.g. EL8's 8.5.0) can be used. Older gcc causes RCU code (rcu_dereference and rcu_access_pointer) to dereference what should just be an opaque pointer with its use of typeof. So without the dummy definition compiling with older gcc fails. Link: https://lore.kernel.org/all/Zsyhco1OrOI_uSbd@kernel.org/ Fixes: 55a9742d02eff ("nfs: cache all open LOCALIO nfsd_file(s) in client") Signed-off-by: Mike Snitzer <snitzer@hammerspace.com> --- fs/nfs/localio.c | 8 ++++++++ fs/nfs_common/nfslocalio.c | 8 ++++++++ 2 files changed, 16 insertions(+)