| Message ID | 20240925015624.3817878-1-lihongbo22@huawei.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] fs: ext4: support relative path for `journal_path` in mount option. | expand |
On Wed, Sep 25, 2024 at 09:56:24AM +0800, Hongbo Li wrote: > @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc, > f = getname_kernel(param->string); > if (IS_ERR(f)) > return PTR_ERR(f); > + /* for relative path */ > + if (f->name[0] != '/') > + param->dirfd = AT_FDCWD; Will need to dig around for some context, but this bit definitely makes no sense - dirfd is completely ignored for absolute pathnames, so making that store conditional is pointless.
On 2024/9/25 10:09, Al Viro wrote: > On Wed, Sep 25, 2024 at 09:56:24AM +0800, Hongbo Li wrote: >> @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc, >> f = getname_kernel(param->string); >> if (IS_ERR(f)) >> return PTR_ERR(f); >> + /* for relative path */ >> + if (f->name[0] != '/') >> + param->dirfd = AT_FDCWD; > > Will need to dig around for some context, but this bit definitely makes > no sense - dirfd is completely ignored for absolute pathnames, so making > that store conditional is pointless. > Only do it for relative path. As mentioned in [1], if the "journal_path" is treated as FSCONFIG_SET_PATH may be better, but mount(8) is passing a string (which uses FSCONFIG_SET_STRING for "journal_path"). For the relative path case, the dirfd should be assigned. [1] https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/ Thanks, Hongbo >
On Wed 25-09-24 09:56:24, Hongbo Li wrote: > The `fs_lookup_param` did not consider the relative path for > block device. When we mount ext4 with `journal_path` option using > relative path, `param->dirfd` was not set which will cause mounting > error. > > This can be reproduced easily like this: > > mke2fs -F -O journal_dev $JOURNAL_DEV -b 4096 100M > mkfs.ext4 -F -J device=$JOURNAL_DEV -b 4096 $FS_DEV > cd /dev; mount -t ext4 -o journal_path=`basename $JOURNAL_DEV` $FS_DEV $MNT > > Fixes: 461c3af045d3 ("ext4: Change handle_mount_opt() to use fs_parameter") > Suggested-by: Christian Brauner <brauner@kernel.org> > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- > v2: > - Change the journal_path parameter as string not bdev, and > determine the relative path situation inside fs_lookup_param. > - Add Suggested-by. > > v1: https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/ > --- > fs/ext4/super.c | 4 ++-- > fs/fs_parser.c | 3 +++ > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 16a4ce704460..cd23536ce46e 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1744,7 +1744,7 @@ static const struct fs_parameter_spec ext4_param_specs[] = { > fsparam_u32 ("min_batch_time", Opt_min_batch_time), > fsparam_u32 ("max_batch_time", Opt_max_batch_time), > fsparam_u32 ("journal_dev", Opt_journal_dev), > - fsparam_bdev ("journal_path", Opt_journal_path), > + fsparam_string ("journal_path", Opt_journal_path), Why did you change this? As far as I can see the only effect would be that empty path will not be allowed (which makes sense) but that seems like an independent change which would deserve a comment in the changelog? Or am I missing something? > fsparam_flag ("journal_checksum", Opt_journal_checksum), > fsparam_flag ("nojournal_checksum", Opt_nojournal_checksum), > fsparam_flag ("journal_async_commit",Opt_journal_async_commit), > @@ -2301,7 +2301,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param) > return -EINVAL; > } > > - error = fs_lookup_param(fc, param, 1, LOOKUP_FOLLOW, &path); > + error = fs_lookup_param(fc, param, true, LOOKUP_FOLLOW, &path); > if (error) { > ext4_msg(NULL, KERN_ERR, "error: could not find " > "journal device path"); > diff --git a/fs/fs_parser.c b/fs/fs_parser.c > index 24727ec34e5a..2ae296764b69 100644 > --- a/fs/fs_parser.c > +++ b/fs/fs_parser.c > @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc, > f = getname_kernel(param->string); > if (IS_ERR(f)) > return PTR_ERR(f); > + /* for relative path */ > + if (f->name[0] != '/') > + param->dirfd = AT_FDCWD; What Al meant is that you can do simply: param->dirfd = AT_FDCWD; and everything will work the same because 'dfd' is ignored for absolute pathnames in path_init(). Honza
On 2024/9/25 15:51, Jan Kara wrote: > On Wed 25-09-24 09:56:24, Hongbo Li wrote: >> The `fs_lookup_param` did not consider the relative path for >> block device. When we mount ext4 with `journal_path` option using >> relative path, `param->dirfd` was not set which will cause mounting >> error. >> >> This can be reproduced easily like this: >> >> mke2fs -F -O journal_dev $JOURNAL_DEV -b 4096 100M >> mkfs.ext4 -F -J device=$JOURNAL_DEV -b 4096 $FS_DEV >> cd /dev; mount -t ext4 -o journal_path=`basename $JOURNAL_DEV` $FS_DEV $MNT >> >> Fixes: 461c3af045d3 ("ext4: Change handle_mount_opt() to use fs_parameter") >> Suggested-by: Christian Brauner <brauner@kernel.org> >> Signed-off-by: Hongbo Li <lihongbo22@huawei.com> >> --- >> v2: >> - Change the journal_path parameter as string not bdev, and >> determine the relative path situation inside fs_lookup_param. >> - Add Suggested-by. >> >> v1: https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/ >> --- >> fs/ext4/super.c | 4 ++-- >> fs/fs_parser.c | 3 +++ >> 2 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index 16a4ce704460..cd23536ce46e 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -1744,7 +1744,7 @@ static const struct fs_parameter_spec ext4_param_specs[] = { >> fsparam_u32 ("min_batch_time", Opt_min_batch_time), >> fsparam_u32 ("max_batch_time", Opt_max_batch_time), >> fsparam_u32 ("journal_dev", Opt_journal_dev), >> - fsparam_bdev ("journal_path", Opt_journal_path), >> + fsparam_string ("journal_path", Opt_journal_path), > > Why did you change this? As far as I can see the only effect would be that > empty path will not be allowed (which makes sense) but that seems like an > independent change which would deserve a comment in the changelog? Or am I > missing something? The fsparam_bdev serves no purpose here, you're right, empty path will not be allowed for `journal_path` option. And all changes are made to fix the issues (journal_path options changed) introduced by the previous new mount api conversion. > >> fsparam_flag ("journal_checksum", Opt_journal_checksum), >> fsparam_flag ("nojournal_checksum", Opt_nojournal_checksum), >> fsparam_flag ("journal_async_commit",Opt_journal_async_commit), >> @@ -2301,7 +2301,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param) >> return -EINVAL; >> } >> >> - error = fs_lookup_param(fc, param, 1, LOOKUP_FOLLOW, &path); >> + error = fs_lookup_param(fc, param, true, LOOKUP_FOLLOW, &path); >> if (error) { >> ext4_msg(NULL, KERN_ERR, "error: could not find " >> "journal device path"); >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c >> index 24727ec34e5a..2ae296764b69 100644 >> --- a/fs/fs_parser.c >> +++ b/fs/fs_parser.c >> @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc, >> f = getname_kernel(param->string); >> if (IS_ERR(f)) >> return PTR_ERR(f); >> + /* for relative path */ >> + if (f->name[0] != '/') >> + param->dirfd = AT_FDCWD; > > What Al meant is that you can do simply: > param->dirfd = AT_FDCWD; > > and everything will work the same because 'dfd' is ignored for absolute > pathnames in path_init(). > ok, seems reasonable. Thanks, Hongbo > Honza
On Wed, Sep 25, 2024 at 09:51:05AM GMT, Jan Kara wrote: > On Wed 25-09-24 09:56:24, Hongbo Li wrote: > > The `fs_lookup_param` did not consider the relative path for > > block device. When we mount ext4 with `journal_path` option using > > relative path, `param->dirfd` was not set which will cause mounting > > error. > > > > This can be reproduced easily like this: > > > > mke2fs -F -O journal_dev $JOURNAL_DEV -b 4096 100M > > mkfs.ext4 -F -J device=$JOURNAL_DEV -b 4096 $FS_DEV > > cd /dev; mount -t ext4 -o journal_path=`basename $JOURNAL_DEV` $FS_DEV $MNT > > > > Fixes: 461c3af045d3 ("ext4: Change handle_mount_opt() to use fs_parameter") > > Suggested-by: Christian Brauner <brauner@kernel.org> > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > > --- > > v2: > > - Change the journal_path parameter as string not bdev, and > > determine the relative path situation inside fs_lookup_param. > > - Add Suggested-by. > > > > v1: https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/ > > --- > > fs/ext4/super.c | 4 ++-- > > fs/fs_parser.c | 3 +++ > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index 16a4ce704460..cd23536ce46e 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -1744,7 +1744,7 @@ static const struct fs_parameter_spec ext4_param_specs[] = { > > fsparam_u32 ("min_batch_time", Opt_min_batch_time), > > fsparam_u32 ("max_batch_time", Opt_max_batch_time), > > fsparam_u32 ("journal_dev", Opt_journal_dev), > > - fsparam_bdev ("journal_path", Opt_journal_path), > > + fsparam_string ("journal_path", Opt_journal_path), > > Why did you change this? As far as I can see the only effect would be that > empty path will not be allowed (which makes sense) but that seems like an > independent change which would deserve a comment in the changelog? Or am I > missing something? I'll drop the ext4 bit as that can be done independently drop the conditional.
On Wed, 25 Sep 2024 09:56:24 +0800, Hongbo Li wrote: > The `fs_lookup_param` did not consider the relative path for > block device. When we mount ext4 with `journal_path` option using > relative path, `param->dirfd` was not set which will cause mounting > error. > > This can be reproduced easily like this: > > [...] Applied to the vfs.misc.v6.13 branch of the vfs/vfs.git tree. Patches in the vfs.misc.v6.13 branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.misc.v6.13 [1/1] fs: ext4: support relative path for `journal_path` in mount option. https://git.kernel.org/vfs/vfs/c/457f7b53e736
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 16a4ce704460..cd23536ce46e 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1744,7 +1744,7 @@ static const struct fs_parameter_spec ext4_param_specs[] = { fsparam_u32 ("min_batch_time", Opt_min_batch_time), fsparam_u32 ("max_batch_time", Opt_max_batch_time), fsparam_u32 ("journal_dev", Opt_journal_dev), - fsparam_bdev ("journal_path", Opt_journal_path), + fsparam_string ("journal_path", Opt_journal_path), fsparam_flag ("journal_checksum", Opt_journal_checksum), fsparam_flag ("nojournal_checksum", Opt_nojournal_checksum), fsparam_flag ("journal_async_commit",Opt_journal_async_commit), @@ -2301,7 +2301,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param) return -EINVAL; } - error = fs_lookup_param(fc, param, 1, LOOKUP_FOLLOW, &path); + error = fs_lookup_param(fc, param, true, LOOKUP_FOLLOW, &path); if (error) { ext4_msg(NULL, KERN_ERR, "error: could not find " "journal device path"); diff --git a/fs/fs_parser.c b/fs/fs_parser.c index 24727ec34e5a..2ae296764b69 100644 --- a/fs/fs_parser.c +++ b/fs/fs_parser.c @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc, f = getname_kernel(param->string); if (IS_ERR(f)) return PTR_ERR(f); + /* for relative path */ + if (f->name[0] != '/') + param->dirfd = AT_FDCWD; put_f = true; break; case fs_value_is_filename:
The `fs_lookup_param` did not consider the relative path for block device. When we mount ext4 with `journal_path` option using relative path, `param->dirfd` was not set which will cause mounting error. This can be reproduced easily like this: mke2fs -F -O journal_dev $JOURNAL_DEV -b 4096 100M mkfs.ext4 -F -J device=$JOURNAL_DEV -b 4096 $FS_DEV cd /dev; mount -t ext4 -o journal_path=`basename $JOURNAL_DEV` $FS_DEV $MNT Fixes: 461c3af045d3 ("ext4: Change handle_mount_opt() to use fs_parameter") Suggested-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Hongbo Li <lihongbo22@huawei.com> --- v2: - Change the journal_path parameter as string not bdev, and determine the relative path situation inside fs_lookup_param. - Add Suggested-by. v1: https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/ --- fs/ext4/super.c | 4 ++-- fs/fs_parser.c | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-)