| Message ID | 20190802123956.2450-2-sergey.senozhatsky@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] i915: convert to new mount API | expand |
Quoting Sergey Senozhatsky (2019-08-02 13:39:56) > put_filesystem() before i915_gemfs_init() deals with > kern_mount() error. > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > --- > drivers/gpu/drm/i915/gem/i915_gemfs.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c > index cf05ba72df9d..d437188d1736 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c > +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c > @@ -24,8 +24,10 @@ int i915_gemfs_init(struct drm_i915_private *i915) > return -ENODEV; > > gemfs = kern_mount(type); Looking around, it looks like we always need to drop type after mounting. Should the put_filesystem(type); be here instead? Anyway, nice catch. > - if (IS_ERR(gemfs)) > + if (IS_ERR(gemfs)) { > + put_filesystem(type); > return PTR_ERR(gemfs); > + } > > /* > * Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most > -- > 2.22.0 >
Quoting Chris Wilson (2019-08-02 13:58:36) > Quoting Sergey Senozhatsky (2019-08-02 13:39:56) > > put_filesystem() before i915_gemfs_init() deals with > > kern_mount() error. > > > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gemfs.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c > > index cf05ba72df9d..d437188d1736 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c > > @@ -24,8 +24,10 @@ int i915_gemfs_init(struct drm_i915_private *i915) > > return -ENODEV; > > > > gemfs = kern_mount(type); > > Looking around, it looks like we always need to drop type after > mounting. Should the > put_filesystem(type); > be here instead? > > Anyway, nice catch. Sigh. put_filesystem() is part of fs internals. I'd be tempted to add static void put_filesystem(struct file_system_type *fs) { module_put(fs->owner); } and cc that for stable. And send a patch to add EXPORT_SYMBOL and queue it for removal. Or just ignore the stable@ since it's unlikely to be ever met in the wild and just request the EXPORT_SYMBOL. -Chris
On (08/02/19 14:10), Chris Wilson wrote: > > > drivers/gpu/drm/i915/gem/i915_gemfs.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c > > > index cf05ba72df9d..d437188d1736 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c > > > @@ -24,8 +24,10 @@ int i915_gemfs_init(struct drm_i915_private *i915) > > > return -ENODEV; > > > > > > gemfs = kern_mount(type); > > > > Looking around, it looks like we always need to drop type after > > mounting. Should the > > put_filesystem(type); > > be here instead? > > > > Anyway, nice catch. > > Sigh. put_filesystem() is part of fs internals. I'd be tempted to add Good catch! So we can switch to vfs_kern_mount(), I guess, but pass different options, depending on has_transparent_hugepage(). -ss
On (08/02/19 22:15), Sergey Senozhatsky wrote: [..] > > > Looking around, it looks like we always need to drop type after > > > mounting. Should the > > > put_filesystem(type); > > > be here instead? > > > > > > Anyway, nice catch. > > > > Sigh. put_filesystem() is part of fs internals. I'd be tempted to add > > Good catch! > > So we can switch to vfs_kern_mount(), I guess, but pass different options, > depending on has_transparent_hugepage(). Hmm. This doesn't look exactly right. It appears that vfs_kern_mount() has a slightly different purpose. It's for drivers which register their own fstype and fs_context/sb callbacks. A typical usage would be static struct file_system_type nfsd_fs_type = { .owner→ → = THIS_MODULE, .name→ → = "nfsd", .init_fs_context = nfsd_init_fs_context, .kill_sb→ = nfsd_umount, }; MODULE_ALIAS_FS("nfsd"); vfs_kern_mount(&nfsd_fs_type, SB_KERNMOUNT, "nfsd", NULL); i915 is a different beast, it just wants to mount fs and reconfigure it, it doesn't want to be an fs. So it seems that current kern_mount() is actually right. Maybe we need to export put_filesystem() instead. -ss
Quoting Sergey Senozhatsky (2019-08-02 14:35:03) > On (08/02/19 22:15), Sergey Senozhatsky wrote: > [..] > > > > Looking around, it looks like we always need to drop type after > > > > mounting. Should the > > > > put_filesystem(type); > > > > be here instead? > > > > > > > > Anyway, nice catch. > > > > > > Sigh. put_filesystem() is part of fs internals. I'd be tempted to add > > > > Good catch! > > > > So we can switch to vfs_kern_mount(), I guess, but pass different options, > > depending on has_transparent_hugepage(). > > Hmm. This doesn't look exactly right. It appears that vfs_kern_mount() > has a slightly different purpose. It's for drivers which register their > own fstype and fs_context/sb callbacks. A typical usage would be > > static struct file_system_type nfsd_fs_type = { > .owner→ → = THIS_MODULE, > .name→ → = "nfsd", > .init_fs_context = nfsd_init_fs_context, > .kill_sb→ = nfsd_umount, > }; > MODULE_ALIAS_FS("nfsd"); > > vfs_kern_mount(&nfsd_fs_type, SB_KERNMOUNT, "nfsd", NULL); > > i915 is a different beast, it just wants to mount fs and reconfigure > it, it doesn't want to be an fs. So it seems that current kern_mount() > is actually right. struct vfsmount *kern_mount(struct file_system_type *type) { struct vfsmount *mnt; mnt = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL); if (!IS_ERR(mnt)) { /* * it is a longterm mount, don't release mnt until * we unmount before file sys is unregistered */ real_mount(mnt)->mnt_ns = MNT_NS_INTERNAL; } return mnt; } With the exception of fiddling with MNT_NS_INTERNAL, it seems amenable for our needs. -Chris
On (08/02/19 14:41), Chris Wilson wrote: [..] > struct vfsmount *kern_mount(struct file_system_type *type) > { > struct vfsmount *mnt; > mnt = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL); > if (!IS_ERR(mnt)) { > /* > * it is a longterm mount, don't release mnt until > * we unmount before file sys is unregistered > */ > real_mount(mnt)->mnt_ns = MNT_NS_INTERNAL; > } > return mnt; > } > > With the exception of fiddling with MNT_NS_INTERNAL, it seems > amenable for our needs. Sorry, not sure I understand. i915 use kern_mount() at the moment. Since we still need to put_filesystem(), I'd probably add one more patch - export put_filesystem() so then we can put_filesystem() in i915. Wonder what would happen if someone would do modprobe i915 rmmod i916 In a loop. So something like this (this is against current patch set). --- drivers/gpu/drm/i915/gem/i915_gemfs.c | 5 ++--- fs/filesystems.c | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index d437188d1736..4ea7a6f750f4 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -24,10 +24,9 @@ int i915_gemfs_init(struct drm_i915_private *i915) return -ENODEV; gemfs = kern_mount(type); - if (IS_ERR(gemfs)) { - put_filesystem(type); + put_filesystem(type); + if (IS_ERR(gemfs)) return PTR_ERR(gemfs); - } /* * Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most diff --git a/fs/filesystems.c b/fs/filesystems.c index 9135646e41ac..4837eda748b5 100644 --- a/fs/filesystems.c +++ b/fs/filesystems.c @@ -45,6 +45,7 @@ void put_filesystem(struct file_system_type *fs) { module_put(fs->owner); } +EXPORT_SYMBOL(put_filesystem); static struct file_system_type **find_filesystem(const char *name, unsigned len) { @@ -280,5 +281,4 @@ struct file_system_type *get_fs_type(const char *name) } return fs; } - EXPORT_SYMBOL(get_fs_type);
Quoting Sergey Senozhatsky (2019-08-02 14:49:55) > On (08/02/19 14:41), Chris Wilson wrote: > [..] > > struct vfsmount *kern_mount(struct file_system_type *type) > > { > > struct vfsmount *mnt; > > mnt = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL); > > if (!IS_ERR(mnt)) { > > /* > > * it is a longterm mount, don't release mnt until > > * we unmount before file sys is unregistered > > */ > > real_mount(mnt)->mnt_ns = MNT_NS_INTERNAL; > > } > > return mnt; > > } > > > > With the exception of fiddling with MNT_NS_INTERNAL, it seems > > amenable for our needs. > > Sorry, not sure I understand. i915 use kern_mount() at the moment. > > Since we still need to put_filesystem(), I'd probably add one more > patch > - export put_filesystem() > > so then we can put_filesystem() in i915. Wonder what would happen > if someone would do > modprobe i915 > rmmod i916 > In a loop. > > So something like this (this is against current patch set). Yes, that's what I in mind. I was thinking of whether we can replace our kern_mount + fc->ops->reconfigure() into a single vfs_kern_mount(), and whether or not that works with get_fs_type("tmpfs"). -Chris
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index cf05ba72df9d..d437188d1736 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -24,8 +24,10 @@ int i915_gemfs_init(struct drm_i915_private *i915) return -ENODEV; gemfs = kern_mount(type); - if (IS_ERR(gemfs)) + if (IS_ERR(gemfs)) { + put_filesystem(type); return PTR_ERR(gemfs); + } /* * Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most
put_filesystem() before i915_gemfs_init() deals with kern_mount() error. Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> --- drivers/gpu/drm/i915/gem/i915_gemfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)