This series attempts to fix a reported exception with mmap
on newer kernels.
-- original regression report --
TL;DR: mmap() seems to be broken on 9pfs on Linux 6.4. setting
"rootflags=ignoreqv" fixes it as well, but it feels like a regression.
I'm tracking down an issue which recently turned up in DistroKit [1] (an
embedded Linux distro based on the ptxdist build system). The issue was a bit
uggly, as my CI didn't find it (systems boot up normally after a while, and I
only use 9p for virtual qemu machines, while most of the test farm is real
hardware).
The qemu machine in question is qemu-system-arm, emulating an ARM v7a machine.
When starting the systems interactively, I get a lot of error output from
ldconfig, like this:
[ 17.412964] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libgcc_s.so.1.
[ 17.418851] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libstdc++.so.
[ 17.425009] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libstdc++.so.6.0.30.
[ 17.436671] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libstdc++.so.6.
[ 17.448451] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libatomic.so.
[ 17.456418] systemd-rc-once[127]: ldconfig: Cannot mmap file /lib/libatomic.so.1.2.0.
...
Running ldconfig with strace shows this, for all libraries::
| statx(AT_FDCWD, "/lib/libnm.so.0", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW|AT_NO_AUTOMOUNT, STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFLNK|0777, stx_size=14, ...}) = 0
| statx(AT_FDCWD, "/lib/libnm.so.0", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT, STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|064 4, stx_size=862228, ...}) = 0
| openat(AT_FDCWD, "/lib/libnm.so.0", O_RDONLY|O_LARGEFILE) = 4
| statx(4, "", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT|AT_EMPTY_PATH, STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|0644, stx_s ize=862228, ...}) = 0
| mmap2(NULL, 862228, PROT_READ, MAP_SHARED, 4, 0) = -1 ENODEV (No such device)
| write(2, "ldconfig: ", 10ldconfig: ) = 10
| write(2, "Cannot mmap file /lib/libnm.so.0"..., 34Cannot mmap file /lib/libnm.so.0.) = 34
| write(2, "\n", 1) = 1
| close(4) = 0
I could track down the breakage to
1543b4c5071c54d76aad7a7a26a6e43082269b0c
My test setup has, in addition to the patch above, the following patches also
reverted on top of a vanilla 6.4 kernel:
4eb3117888a923f6b9b1ad2dd093641c49a63ae5
21e26d5e54ab7cfe6b488fd27d4d70956d07e03b
as 1543b cannot be reverted without those; however, the effect only goes away
when I also revert 1543b. The kernel has no other patches applied, only these
three reverts.
-- end bug report --
Reported-by: Robert Schwebel <[email protected]>
Signed-off-by: Eric Van Hensbergen <[email protected]>
---
Eric Van Hensbergen (3):
fs/9p: remove unecessary and overrestrictive check
fs/9p: fix typo in comparison logic for cache mode
fs/9p: fix type mismatch in file cache mode helper
fs/9p/fid.h | 6 +++---
fs/9p/vfs_file.c | 2 --
2 files changed, 3 insertions(+), 5 deletions(-)
---
base-commit: 95f41d87810083d8b3dedcce46a4e356cf4a9673
change-id: 20230716-fixes-overly-restrictive-mmap-30a23501e787
Best regards,
--
Eric Van Hensbergen <[email protected]>
There were two flags which had incorrect type in the
paramaters of the file cache mode helper function.
Signed-off-by: Eric Van Hensbergen <[email protected]>
---
fs/9p/fid.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/9p/fid.h b/fs/9p/fid.h
index 297c2c377e3dd..29281b7c38870 100644
--- a/fs/9p/fid.h
+++ b/fs/9p/fid.h
@@ -46,8 +46,8 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)
* NOTE: these are set after open so only reflect 9p client not
* underlying file system on server.
*/
-static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
- int s_cache, unsigned int f_flags)
+static inline void v9fs_fid_add_modes(struct p9_fid *fid, unsigned int s_flags,
+ unsigned int s_cache, unsigned int f_flags)
{
if (fid->qid.type != P9_QTFILE)
return;
--
2.39.2
There appears to be a typo in the comparison statement for the logic
which sets a file's cache mode based on mount flags.
Signed-off-by: Eric Van Hensbergen <[email protected]>
---
fs/9p/fid.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/9p/fid.h b/fs/9p/fid.h
index 0c51889a60b33..297c2c377e3dd 100644
--- a/fs/9p/fid.h
+++ b/fs/9p/fid.h
@@ -57,7 +57,7 @@ static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
(s_flags & V9FS_DIRECT_IO) || (f_flags & O_DIRECT)) {
fid->mode |= P9L_DIRECT; /* no read or write cache */
} else if ((!(s_cache & CACHE_WRITEBACK)) ||
- (f_flags & O_DSYNC) | (s_flags & V9FS_SYNC)) {
+ (f_flags & O_DSYNC) || (s_flags & V9FS_SYNC)) {
fid->mode |= P9L_NOWRITECACHE;
}
}
--
2.39.2
This eliminates a check for shared that was overrestrictive and
duplicated a check in generic_file_readonly_mmap.
Signed-off-by: Eric Van Hensbergen <[email protected]>
---
fs/9p/vfs_file.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index 2996fb00387fa..bda3abd6646b8 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -506,8 +506,6 @@ v9fs_file_mmap(struct file *filp, struct vm_area_struct *vma)
if (!(v9ses->cache & CACHE_WRITEBACK)) {
p9_debug(P9_DEBUG_CACHE, "(no mmap mode)");
- if (vma->vm_flags & VM_MAYSHARE)
- return -ENODEV;
invalidate_inode_pages2(filp->f_mapping);
return generic_file_readonly_mmap(filp, vma);
}
--
2.39.2
Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:01PM +0000:
> There appears to be a typo in the comparison statement for the logic
> which sets a file's cache mode based on mount flags.
Shouldn't break anything, but good fix nevertheless, thanks!
Reviewed-by: Dominique Martinet <[email protected]>
>
> Signed-off-by: Eric Van Hensbergen <[email protected]>
> ---
> fs/9p/fid.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> index 0c51889a60b33..297c2c377e3dd 100644
> --- a/fs/9p/fid.h
> +++ b/fs/9p/fid.h
> @@ -57,7 +57,7 @@ static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
> (s_flags & V9FS_DIRECT_IO) || (f_flags & O_DIRECT)) {
> fid->mode |= P9L_DIRECT; /* no read or write cache */
> } else if ((!(s_cache & CACHE_WRITEBACK)) ||
> - (f_flags & O_DSYNC) | (s_flags & V9FS_SYNC)) {
> + (f_flags & O_DSYNC) || (s_flags & V9FS_SYNC)) {
> fid->mode |= P9L_NOWRITECACHE;
> }
> }
>
--
Dominique Martinet | Asmadeus
Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:00PM +0000:
> This eliminates a check for shared that was overrestrictive and
> duplicated a check in generic_file_readonly_mmap.
generic_file_readonly_mmap checks for VM_SHARED + VM_MAYWRITE,
so it isn't exactly "duplicating" the check.. But I agree we don't need
it; we used to have the mmap op be generic_file_readonly_mmap directly
previously.
I'd argue we don't need to invalidate inode pages either if there is no
writeback cache, there shouldn't be anything in it? but that can be done
separately, and extra invalidation won't bring harm anyway.
Reviewed-by: Dominique Martinet <[email protected]>
>
> Signed-off-by: Eric Van Hensbergen <[email protected]>
> ---
> fs/9p/vfs_file.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 2996fb00387fa..bda3abd6646b8 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -506,8 +506,6 @@ v9fs_file_mmap(struct file *filp, struct vm_area_struct *vma)
>
> if (!(v9ses->cache & CACHE_WRITEBACK)) {
> p9_debug(P9_DEBUG_CACHE, "(no mmap mode)");
> - if (vma->vm_flags & VM_MAYSHARE)
> - return -ENODEV;
> invalidate_inode_pages2(filp->f_mapping);
> return generic_file_readonly_mmap(filp, vma);
> }
>
--
Dominique Martinet | Asmadeus
Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:02PM +0000:
> There were two flags which had incorrect type in the
> paramaters of the file cache mode helper function.
Checked the parameters passed (v9ses->cache, file->f_flags and a flags
parent function parameters) were all correctly unsigned:
Reviewed-by: Dominique Martinet <[email protected]>
>
> Signed-off-by: Eric Van Hensbergen <[email protected]>
> ---
> fs/9p/fid.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> index 297c2c377e3dd..29281b7c38870 100644
> --- a/fs/9p/fid.h
> +++ b/fs/9p/fid.h
> @@ -46,8 +46,8 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)
> * NOTE: these are set after open so only reflect 9p client not
> * underlying file system on server.
> */
> -static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
> - int s_cache, unsigned int f_flags)
> +static inline void v9fs_fid_add_modes(struct p9_fid *fid, unsigned int s_flags,
> + unsigned int s_cache, unsigned int f_flags)
> {
> if (fid->qid.type != P9_QTFILE)
> return;
>
--
Dominique Martinet | Asmadeus
On Monday, July 17, 2023 6:29:02 PM CEST Eric Van Hensbergen wrote:
> There were two flags which had incorrect type in the
> paramaters of the file cache mode helper function.
1. typo: "parameters"
2. I would be more precise and actually name the two flags in question in the
commit log.
> Signed-off-by: Eric Van Hensbergen <[email protected]>
> ---
> fs/9p/fid.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> index 297c2c377e3dd..29281b7c38870 100644
> --- a/fs/9p/fid.h
> +++ b/fs/9p/fid.h
> @@ -46,8 +46,8 @@ static inline struct p9_fid *v9fs_fid_clone(struct dentry *dentry)
> * NOTE: these are set after open so only reflect 9p client not
> * underlying file system on server.
> */
> -static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
> - int s_cache, unsigned int f_flags)
> +static inline void v9fs_fid_add_modes(struct p9_fid *fid, unsigned int s_flags,
> + unsigned int s_cache, unsigned int f_flags)
> {
> if (fid->qid.type != P9_QTFILE)
> return;
>
>
On Tuesday, July 18, 2023 4:48:53 AM CEST Dominique Martinet wrote:
> Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:01PM +0000:
> > There appears to be a typo in the comparison statement for the logic
> > which sets a file's cache mode based on mount flags.
>
> Shouldn't break anything, but good fix nevertheless, thanks!
>
> Reviewed-by: Dominique Martinet <[email protected]>
Right, at least AFAICS I would not expect any visible behaviour change by this
patch at all. So this patch is probably just a formal fix.
BTW there are a bunch of unnecessary braces in this function:
(!s_cache) -> !s_cache
(fid->qid.version == 0) -> fid->qid.version == 0
(!(s_cache & CACHE_WRITEBACK)) -> !(s_cache & CACHE_WRITEBACK)
These could be wiped in a separate patch as well. Anyway ...
Reviewed-by: Christian Schoenebeck <[email protected]>
> >
> > Signed-off-by: Eric Van Hensbergen <[email protected]>
> > ---
> > fs/9p/fid.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/9p/fid.h b/fs/9p/fid.h
> > index 0c51889a60b33..297c2c377e3dd 100644
> > --- a/fs/9p/fid.h
> > +++ b/fs/9p/fid.h
> > @@ -57,7 +57,7 @@ static inline void v9fs_fid_add_modes(struct p9_fid *fid, int s_flags,
> > (s_flags & V9FS_DIRECT_IO) || (f_flags & O_DIRECT)) {
> > fid->mode |= P9L_DIRECT; /* no read or write cache */
> > } else if ((!(s_cache & CACHE_WRITEBACK)) ||
> > - (f_flags & O_DSYNC) | (s_flags & V9FS_SYNC)) {
> > + (f_flags & O_DSYNC) || (s_flags & V9FS_SYNC)) {
> > fid->mode |= P9L_NOWRITECACHE;
> > }
> > }
> >
>
>
On Tuesday, July 18, 2023 4:45:25 AM CEST Dominique Martinet wrote:
> Eric Van Hensbergen wrote on Mon, Jul 17, 2023 at 04:29:00PM +0000:
> > This eliminates a check for shared that was overrestrictive and
> > duplicated a check in generic_file_readonly_mmap.
>
> generic_file_readonly_mmap checks for VM_SHARED + VM_MAYWRITE,
> so it isn't exactly "duplicating" the check.. But I agree we don't need
> it; we used to have the mmap op be generic_file_readonly_mmap directly
> previously.
It should be removed from the commit log then though.
> I'd argue we don't need to invalidate inode pages either if there is no
> writeback cache, there shouldn't be anything in it? but that can be done
> separately, and extra invalidation won't bring harm anyway.
Unnecessary performance penalty? So I would drop that in a separate patch.
> Reviewed-by: Dominique Martinet <[email protected]>
Feel free to add my RB as well:
Reviewed-by: Christian Schoenebeck <[email protected]>
> >
> > Signed-off-by: Eric Van Hensbergen <[email protected]>
> > ---
> > fs/9p/vfs_file.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> > index 2996fb00387fa..bda3abd6646b8 100644
> > --- a/fs/9p/vfs_file.c
> > +++ b/fs/9p/vfs_file.c
> > @@ -506,8 +506,6 @@ v9fs_file_mmap(struct file *filp, struct vm_area_struct *vma)
> >
> > if (!(v9ses->cache & CACHE_WRITEBACK)) {
> > p9_debug(P9_DEBUG_CACHE, "(no mmap mode)");
> > - if (vma->vm_flags & VM_MAYSHARE)
> > - return -ENODEV;
> > invalidate_inode_pages2(filp->f_mapping);
> > return generic_file_readonly_mmap(filp, vma);
> > }
> >
>
>
On 17.07.23 18:28, Eric Van Hensbergen wrote:
> This series attempts to fix a reported exception with mmap
> on newer kernels.
>
> -- original regression report --
>
> TL;DR: mmap() seems to be broken on 9pfs on Linux 6.4. setting
> "rootflags=ignoreqv" fixes it as well, but it feels like a regression.
>
> [...]
>
> I could track down the breakage to
>
> 1543b4c5071c54d76aad7a7a26a6e43082269b0c
>
> My test setup has, in addition to the patch above, the following patches also
> reverted on top of a vanilla 6.4 kernel:
>
> 4eb3117888a923f6b9b1ad2dd093641c49a63ae5
> 21e26d5e54ab7cfe6b488fd27d4d70956d07e03b
>
> as 1543b cannot be reverted without those; however, the effect only goes away
> when I also revert 1543b. The kernel has no other patches applied, only these
> three reverts.
>
> -- end bug report --
>
> Reported-by: Robert Schwebel <[email protected]>
This tag afaics should be in some or all of the commits -- together with
a Link: or Closes: tag to the report
(https://lore.kernel.org/v9fs/ZK25XZ%2BGpR3KHIB%[email protected]/ ).
'Fixes:' tags would seem appropriate here as well. And to get this fixed
in Linux 6.4.y. a "Cc: <stable..." would be great as well.
See Documentation/handling-regressions.rst (
https://docs.kernel.org/process/handling-regressions.html ) and the
links to Documentation/process/submitting-patches.rst or
Documentation/process/5.Posting.rst for details.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
#regzbot ^backmonitor:
https://lore.kernel.org/v9fs/ZK25XZ%2BGpR3KHIB%[email protected]/