| Message ID | 20241008095606.990466-1-hsiangkao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] fs/super.c: introduce get_tree_bdev_by_dev() | expand |
On Tue, Oct 08, 2024 at 05:56:05PM +0800, Gao Xiang wrote: > As Allison reported [1], currently get_tree_bdev() will store > "Can't lookup blockdev" error message. Although it makes sense for > pure bdev-based fses, this message may mislead users who try to use > EROFS file-backed mounts since get_tree_nodev() is used as a fallback > then. > > Add get_tree_bdev_by_dev() to specify a device number explicitly > instead of the hardcoded fc->source as mentioned in [2], there are > other benefits like: > - Filesystems can have other ways to get a bdev-based sb > in addition to the current hard-coded source path; > > - Pseudo-filesystems can utilize this method to generate a > filesystem from given device numbers too. > > - Like get_tree_nodev(), it doesn't strictly tie to fc->source > either. Do you have concrete plans for any of those? If so send pointers. Otherwise just passing a quiet flag of some form feels like a much saner interface.
Hi Christoph, On 2024/10/8 19:49, Christoph Hellwig wrote: > On Tue, Oct 08, 2024 at 05:56:05PM +0800, Gao Xiang wrote: >> As Allison reported [1], currently get_tree_bdev() will store >> "Can't lookup blockdev" error message. Although it makes sense for >> pure bdev-based fses, this message may mislead users who try to use >> EROFS file-backed mounts since get_tree_nodev() is used as a fallback >> then. >> >> Add get_tree_bdev_by_dev() to specify a device number explicitly >> instead of the hardcoded fc->source as mentioned in [2], there are >> other benefits like: >> - Filesystems can have other ways to get a bdev-based sb >> in addition to the current hard-coded source path; >> >> - Pseudo-filesystems can utilize this method to generate a >> filesystem from given device numbers too. >> >> - Like get_tree_nodev(), it doesn't strictly tie to fc->source >> either. > > Do you have concrete plans for any of those? If so send pointers. Thanks for your comment. I don't have any pointer for now, just listing the potential use cases for reference. But the error message out of get_tree_bdev() is inflexible and IMHO it's too coupled to `fc->source`. > Otherwise just passing a quiet flag of some form feels like a much > saner interface. I'm fine with this way, but that will be a treewide change, I will send out a version with a flag later. Thanks, Gao Xiang
On Tue, Oct 08, 2024 at 08:10:45PM +0800, Gao Xiang wrote: > But the error message out of get_tree_bdev() is inflexible and > IMHO it's too coupled to `fc->source`. > > > Otherwise just passing a quiet flag of some form feels like a much > > saner interface. > > I'm fine with this way, but that will be a treewide change, I > will send out a version with a flag later. I'd probably just add a get_tree_bdev_flags and pass 0 flags from get_tree_bdev.
On 2024/10/8 20:23, Christoph Hellwig wrote: > On Tue, Oct 08, 2024 at 08:10:45PM +0800, Gao Xiang wrote: >> But the error message out of get_tree_bdev() is inflexible and >> IMHO it's too coupled to `fc->source`. >> >>> Otherwise just passing a quiet flag of some form feels like a much >>> saner interface. >> >> I'm fine with this way, but that will be a treewide change, I >> will send out a version with a flag later. > > I'd probably just add a get_tree_bdev_flags and pass 0 flags from > get_tree_bdev. how about int get_tree_bdev_flags(struct fs_context *fc, int (*fill_super)(struct super_block *, struct fs_context *), bool quiet) for now? it can be turned into `int flags` if other needs are shown later (and I don't need to define an enum.) Does it look good to you? if okay, I will follow this way. Thanks, Gao Xiang
On Tue, Oct 08, 2024 at 08:33:27PM +0800, Gao Xiang wrote: > how about > int get_tree_bdev_flags(struct fs_context *fc, > int (*fill_super)(struct super_block *, > struct fs_context *), bool quiet) > > for now? it can be turned into `int flags` if other needs > are shown later (and I don't need to define an enum.) I'd pass an unsigned int flags with a clearly spellt out (and extensible) flags namespae.
On 2024/10/8 20:55, Christoph Hellwig wrote: > On Tue, Oct 08, 2024 at 08:33:27PM +0800, Gao Xiang wrote: >> how about >> int get_tree_bdev_flags(struct fs_context *fc, >> int (*fill_super)(struct super_block *, >> struct fs_context *), bool quiet) >> >> for now? it can be turned into `int flags` if other needs >> are shown later (and I don't need to define an enum.) > > I'd pass an unsigned int flags with a clearly spellt out (and > extensible) flags namespae. okay, got it. Thanks, Gao Xiang
diff --git a/fs/super.c b/fs/super.c index 1db230432960..8cc8350b9ba6 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1596,26 +1596,17 @@ int setup_bdev_super(struct super_block *sb, int sb_flags, EXPORT_SYMBOL_GPL(setup_bdev_super); /** - * get_tree_bdev - Get a superblock based on a single block device + * get_tree_bdev_by_dev - Get a bdev-based superblock with a given device number * @fc: The filesystem context holding the parameters * @fill_super: Helper to initialise a new superblock + * @dev: The device number indicating the target block device */ -int get_tree_bdev(struct fs_context *fc, +int get_tree_bdev_by_dev(struct fs_context *fc, int (*fill_super)(struct super_block *, - struct fs_context *)) + struct fs_context *), dev_t dev) { struct super_block *s; int error = 0; - dev_t dev; - - if (!fc->source) - return invalf(fc, "No source specified"); - - error = lookup_bdev(fc->source, &dev); - if (error) { - errorf(fc, "%s: Can't lookup blockdev", fc->source); - return error; - } fc->sb_flags |= SB_NOSEC; s = sget_dev(fc, dev); @@ -1644,6 +1635,30 @@ int get_tree_bdev(struct fs_context *fc, fc->root = dget(s->s_root); return 0; } +EXPORT_SYMBOL_GPL(get_tree_bdev_by_dev); + +/** + * get_tree_bdev - Get a superblock based on a single block device + * @fc: The filesystem context holding the parameters + * @fill_super: Helper to initialise a new superblock + */ +int get_tree_bdev(struct fs_context *fc, + int (*fill_super)(struct super_block *, + struct fs_context *)) +{ + int error; + dev_t dev; + + if (!fc->source) + return invalf(fc, "No source specified"); + + error = lookup_bdev(fc->source, &dev); + if (error) { + errorf(fc, "%s: Can't lookup blockdev", fc->source); + return error; + } + return get_tree_bdev_by_dev(fc, fill_super, dev); +} EXPORT_SYMBOL(get_tree_bdev); static int test_bdev_super(struct super_block *s, void *data) diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index c13e99cbbf81..54f23589ad5b 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -160,6 +160,9 @@ extern int get_tree_keyed(struct fs_context *fc, int setup_bdev_super(struct super_block *sb, int sb_flags, struct fs_context *fc); +int get_tree_bdev_by_dev(struct fs_context *fc, + int (*fill_super)(struct super_block *sb, + struct fs_context *fc), dev_t dev); extern int get_tree_bdev(struct fs_context *fc, int (*fill_super)(struct super_block *sb, struct fs_context *fc));
As Allison reported [1], currently get_tree_bdev() will store "Can't lookup blockdev" error message. Although it makes sense for pure bdev-based fses, this message may mislead users who try to use EROFS file-backed mounts since get_tree_nodev() is used as a fallback then. Add get_tree_bdev_by_dev() to specify a device number explicitly instead of the hardcoded fc->source as mentioned in [2], there are other benefits like: - Filesystems can have other ways to get a bdev-based sb in addition to the current hard-coded source path; - Pseudo-filesystems can utilize this method to generate a filesystem from given device numbers too. - Like get_tree_nodev(), it doesn't strictly tie to fc->source either. [1] https://lore.kernel.org/r/CAOYeF9VQ8jKVmpy5Zy9DNhO6xmWSKMB-DO8yvBB0XvBE7=3Ugg@mail.gmail.com [2] https://lore.kernel.org/r/b9565874-7018-46ef-b123-b524a1dffb21@linux.alibaba.com Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com> --- fs/super.c | 41 ++++++++++++++++++++++++++------------ include/linux/fs_context.h | 3 +++ 2 files changed, 31 insertions(+), 13 deletions(-)