We need to commit dirty metadata and pages to disk
before remounting exfat as read-only.
This fixes a failure in xfstests generic/452
Signed-off-by: Hyunchul Lee <[email protected]>
---
fs/exfat/super.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index e650e65536f8..61c6cf240c19 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -693,10 +693,29 @@ static void exfat_free(struct fs_context *fc)
}
}
+static int exfat_reconfigure(struct fs_context *fc)
+{
+ struct super_block *sb = fc->root->d_sb;
+ int ret;
+ bool new_rdonly;
+
+ new_rdonly = fc->sb_flags & SB_RDONLY;
+ if (new_rdonly != sb_rdonly(sb)) {
+ if (new_rdonly) {
+ /* volume flag will be updated in exfat_sync_fs */
+ ret = sync_filesystem(sb);
+ if (ret < 0)
+ return ret;
+ }
+ }
+ return 0;
+}
+
static const struct fs_context_operations exfat_context_ops = {
.parse_param = exfat_parse_param,
.get_tree = exfat_get_tree,
.free = exfat_free,
+ .reconfigure = exfat_reconfigure,
};
static int exfat_init_fs_context(struct fs_context *fc)
--
2.17.1
Allow to change permission masks, allow_utime,
errors. But ignore other options.
Signed-off-by: Hyunchul Lee <[email protected]>
---
fs/exfat/super.c | 40 +++++++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 61c6cf240c19..3c1d47289ba2 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -696,9 +696,13 @@ static void exfat_free(struct fs_context *fc)
static int exfat_reconfigure(struct fs_context *fc)
{
struct super_block *sb = fc->root->d_sb;
+ struct exfat_sb_info *sbi = EXFAT_SB(sb);
+ struct exfat_mount_options *new_opts;
int ret;
bool new_rdonly;
+ new_opts = &((struct exfat_sb_info *)fc->s_fs_info)->options;
+
new_rdonly = fc->sb_flags & SB_RDONLY;
if (new_rdonly != sb_rdonly(sb)) {
if (new_rdonly) {
@@ -708,6 +712,12 @@ static int exfat_reconfigure(struct fs_context *fc)
return ret;
}
}
+
+ /* allow to change these options but ignore others */
+ sbi->options.fs_fmask = new_opts->fs_fmask;
+ sbi->options.fs_dmask = new_opts->fs_dmask;
+ sbi->options.allow_utime = new_opts->allow_utime;
+ sbi->options.errors = new_opts->errors;
return 0;
}
@@ -726,17 +736,25 @@ static int exfat_init_fs_context(struct fs_context *fc)
if (!sbi)
return -ENOMEM;
- mutex_init(&sbi->s_lock);
- ratelimit_state_init(&sbi->ratelimit, DEFAULT_RATELIMIT_INTERVAL,
- DEFAULT_RATELIMIT_BURST);
-
- sbi->options.fs_uid = current_uid();
- sbi->options.fs_gid = current_gid();
- sbi->options.fs_fmask = current->fs->umask;
- sbi->options.fs_dmask = current->fs->umask;
- sbi->options.allow_utime = -1;
- sbi->options.iocharset = exfat_default_iocharset;
- sbi->options.errors = EXFAT_ERRORS_RO;
+ if (fc->root) {
+ /* reconfiguration */
+ memcpy(&sbi->options, &EXFAT_SB(fc->root->d_sb)->options,
+ sizeof(struct exfat_mount_options));
+ sbi->options.iocharset = exfat_default_iocharset;
+ } else {
+ mutex_init(&sbi->s_lock);
+ ratelimit_state_init(&sbi->ratelimit,
+ DEFAULT_RATELIMIT_INTERVAL,
+ DEFAULT_RATELIMIT_BURST);
+
+ sbi->options.fs_uid = current_uid();
+ sbi->options.fs_gid = current_gid();
+ sbi->options.fs_fmask = current->fs->umask;
+ sbi->options.fs_dmask = current->fs->umask;
+ sbi->options.allow_utime = -1;
+ sbi->options.iocharset = exfat_default_iocharset;
+ sbi->options.errors = EXFAT_ERRORS_RO;
+ }
fc->s_fs_info = sbi;
fc->ops = &exfat_context_ops;
--
2.17.1
Hi Hyunchul,
> We need to commit dirty metadata and pages to disk before remounting exfat as read-only.
>
> This fixes a failure in xfstests generic/452
Could you please elaborate more the reason why generic/452 in xfstests failed ?
>
> Signed-off-by: Hyunchul Lee <[email protected]>
> ---
> fs/exfat/super.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index e650e65536f8..61c6cf240c19 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -693,10 +693,29 @@ static void exfat_free(struct fs_context *fc)
> }
> }
>
> +static int exfat_reconfigure(struct fs_context *fc) {
> + struct super_block *sb = fc->root->d_sb;
> + int ret;
int ret = 0;
> + bool new_rdonly;
> +
> + new_rdonly = fc->sb_flags & SB_RDONLY;
> + if (new_rdonly != sb_rdonly(sb)) {
If you modify it like this, would not we need new_rdonly?
if (fc->sb_flags & SB_RDONLY && !sb_rdonly(sb))
> + if (new_rdonly) {
> + /* volume flag will be updated in exfat_sync_fs */
> + ret = sync_filesystem(sb);
> + if (ret < 0)
> + return ret;
I think that this ret check can be removed by using return ret; below ?
> + }
> + }
> + return 0;
return ret;
> +}
> +
> static const struct fs_context_operations exfat_context_ops = {
> .parse_param = exfat_parse_param,
> .get_tree = exfat_get_tree,
> .free = exfat_free,
> + .reconfigure = exfat_reconfigure,
> };
>
> static int exfat_init_fs_context(struct fs_context *fc)
> --
> 2.17.1
> Allow to change permission masks, allow_utime, errors. But ignore other options.
>
> Signed-off-by: Hyunchul Lee <[email protected]>
> ---
> fs/exfat/super.c | 40 +++++++++++++++++++++++++++++-----------
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index 61c6cf240c19..3c1d47289ba2 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -696,9 +696,13 @@ static void exfat_free(struct fs_context *fc) static int
> exfat_reconfigure(struct fs_context *fc) {
> struct super_block *sb = fc->root->d_sb;
> + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> + struct exfat_mount_options *new_opts;
> int ret;
> bool new_rdonly;
>
> + new_opts = &((struct exfat_sb_info *)fc->s_fs_info)->options;
> +
> new_rdonly = fc->sb_flags & SB_RDONLY;
> if (new_rdonly != sb_rdonly(sb)) {
> if (new_rdonly) {
> @@ -708,6 +712,12 @@ static int exfat_reconfigure(struct fs_context *fc)
> return ret;
> }
> }
> +
> + /* allow to change these options but ignore others */
> + sbi->options.fs_fmask = new_opts->fs_fmask;
> + sbi->options.fs_dmask = new_opts->fs_dmask;
> + sbi->options.allow_utime = new_opts->allow_utime;
> + sbi->options.errors = new_opts->errors;
Is there any reason why you allow a few options on remount ?
> return 0;
> }
>
> @@ -726,17 +736,25 @@ static int exfat_init_fs_context(struct fs_context *fc)
> if (!sbi)
> return -ENOMEM;
>
> - mutex_init(&sbi->s_lock);
> - ratelimit_state_init(&sbi->ratelimit, DEFAULT_RATELIMIT_INTERVAL,
> - DEFAULT_RATELIMIT_BURST);
> -
> - sbi->options.fs_uid = current_uid();
> - sbi->options.fs_gid = current_gid();
> - sbi->options.fs_fmask = current->fs->umask;
> - sbi->options.fs_dmask = current->fs->umask;
> - sbi->options.allow_utime = -1;
> - sbi->options.iocharset = exfat_default_iocharset;
> - sbi->options.errors = EXFAT_ERRORS_RO;
> + if (fc->root) {
> + /* reconfiguration */
> + memcpy(&sbi->options, &EXFAT_SB(fc->root->d_sb)->options,
> + sizeof(struct exfat_mount_options));
> + sbi->options.iocharset = exfat_default_iocharset;
> + } else {
> + mutex_init(&sbi->s_lock);
> + ratelimit_state_init(&sbi->ratelimit,
> + DEFAULT_RATELIMIT_INTERVAL,
> + DEFAULT_RATELIMIT_BURST);
> +
> + sbi->options.fs_uid = current_uid();
> + sbi->options.fs_gid = current_gid();
> + sbi->options.fs_fmask = current->fs->umask;
> + sbi->options.fs_dmask = current->fs->umask;
> + sbi->options.allow_utime = -1;
> + sbi->options.iocharset = exfat_default_iocharset;
> + sbi->options.errors = EXFAT_ERRORS_RO;
> + }
>
> fc->s_fs_info = sbi;
> fc->ops = &exfat_context_ops;
> --
> 2.17.1
Hi Namjae,
2020년 6월 15일 (월) 오전 9:14, Namjae Jeon <[email protected]>님이 작성:
>
> Hi Hyunchul,
> > We need to commit dirty metadata and pages to disk before remounting exfat as read-only.
> >
> > This fixes a failure in xfstests generic/452
> Could you please elaborate more the reason why generic/452 in xfstests failed ?
xfstests generic/452 does the following.
cp /bin/ls <exfat>/
mount -o remount,ro <exfat>
the <exfat>/ls file is corrupted, because while exfat is remounted as read-only,
exfat doesn't have a chance to commit metadata and vfs invalidates page
caches in a block device.
I will put this explanation in a commit message.
> >
> > Signed-off-by: Hyunchul Lee <[email protected]>
> > ---
> > fs/exfat/super.c | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/fs/exfat/super.c b/fs/exfat/super.c index e650e65536f8..61c6cf240c19 100644
> > --- a/fs/exfat/super.c
> > +++ b/fs/exfat/super.c
> > @@ -693,10 +693,29 @@ static void exfat_free(struct fs_context *fc)
> > }
> > }
> >
> > +static int exfat_reconfigure(struct fs_context *fc) {
> > + struct super_block *sb = fc->root->d_sb;
> > + int ret;
> int ret = 0;
> > + bool new_rdonly;
> > +
> > + new_rdonly = fc->sb_flags & SB_RDONLY;
> > + if (new_rdonly != sb_rdonly(sb)) {
> If you modify it like this, would not we need new_rdonly?
> if (fc->sb_flags & SB_RDONLY && !sb_rdonly(sb))
>
This condition means that mount options are changed from "rw" to "ro",
or "ro" to "rw".
> > + if (new_rdonly) {
And this condition means these options are changed from "rw" to "ro".
It seems better to change two conditions to the one you suggested, or
remove those. because sync_filesystem returns 0 when the filesystem is
mounted as read-only.
> > + /* volume flag will be updated in exfat_sync_fs */
> > + ret = sync_filesystem(sb);
> > + if (ret < 0)
> > + return ret;
> I think that this ret check can be removed by using return ret; below ?
Okay, I will apply this.
Thank you for your comments!
> > + }
> > + }
> > + return 0;
> return ret;
> > +}
> > +
> > static const struct fs_context_operations exfat_context_ops = {
> > .parse_param = exfat_parse_param,
> > .get_tree = exfat_get_tree,
> > .free = exfat_free,
> > + .reconfigure = exfat_reconfigure,
> > };
> >
> > static int exfat_init_fs_context(struct fs_context *fc)
> > --
> > 2.17.1
>
>
2020년 6월 15일 (월) 오전 9:18, Namjae Jeon <[email protected]>님이 작성:
>
> > Allow to change permission masks, allow_utime, errors. But ignore other options.
> >
> > Signed-off-by: Hyunchul Lee <[email protected]>
> > ---
> > fs/exfat/super.c | 40 +++++++++++++++++++++++++++++-----------
> > 1 file changed, 29 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/exfat/super.c b/fs/exfat/super.c index 61c6cf240c19..3c1d47289ba2 100644
> > --- a/fs/exfat/super.c
> > +++ b/fs/exfat/super.c
> > @@ -696,9 +696,13 @@ static void exfat_free(struct fs_context *fc) static int
> > exfat_reconfigure(struct fs_context *fc) {
> > struct super_block *sb = fc->root->d_sb;
> > + struct exfat_sb_info *sbi = EXFAT_SB(sb);
> > + struct exfat_mount_options *new_opts;
> > int ret;
> > bool new_rdonly;
> >
> > + new_opts = &((struct exfat_sb_info *)fc->s_fs_info)->options;
> > +
> > new_rdonly = fc->sb_flags & SB_RDONLY;
> > if (new_rdonly != sb_rdonly(sb)) {
> > if (new_rdonly) {
> > @@ -708,6 +712,12 @@ static int exfat_reconfigure(struct fs_context *fc)
> > return ret;
> > }
> > }
> > +
> > + /* allow to change these options but ignore others */
> > + sbi->options.fs_fmask = new_opts->fs_fmask;
> > + sbi->options.fs_dmask = new_opts->fs_dmask;
> > + sbi->options.allow_utime = new_opts->allow_utime;
> > + sbi->options.errors = new_opts->errors;
> Is there any reason why you allow a few options on remount ?
while exfat is remounted, inodes are not reclaimed. So I think
changing fs_uid, fs_gid, or time_offset is not impossible.
And I am not sure changing the iocharset is safe.
I am curious about your opinion.
Thanks.
> > return 0;
> > }
> >
> > @@ -726,17 +736,25 @@ static int exfat_init_fs_context(struct fs_context *fc)
> > if (!sbi)
> > return -ENOMEM;
> >
> > - mutex_init(&sbi->s_lock);
> > - ratelimit_state_init(&sbi->ratelimit, DEFAULT_RATELIMIT_INTERVAL,
> > - DEFAULT_RATELIMIT_BURST);
> > -
> > - sbi->options.fs_uid = current_uid();
> > - sbi->options.fs_gid = current_gid();
> > - sbi->options.fs_fmask = current->fs->umask;
> > - sbi->options.fs_dmask = current->fs->umask;
> > - sbi->options.allow_utime = -1;
> > - sbi->options.iocharset = exfat_default_iocharset;
> > - sbi->options.errors = EXFAT_ERRORS_RO;
> > + if (fc->root) {
> > + /* reconfiguration */
> > + memcpy(&sbi->options, &EXFAT_SB(fc->root->d_sb)->options,
> > + sizeof(struct exfat_mount_options));
> > + sbi->options.iocharset = exfat_default_iocharset;
> > + } else {
> > + mutex_init(&sbi->s_lock);
> > + ratelimit_state_init(&sbi->ratelimit,
> > + DEFAULT_RATELIMIT_INTERVAL,
> > + DEFAULT_RATELIMIT_BURST);
> > +
> > + sbi->options.fs_uid = current_uid();
> > + sbi->options.fs_gid = current_gid();
> > + sbi->options.fs_fmask = current->fs->umask;
> > + sbi->options.fs_dmask = current->fs->umask;
> > + sbi->options.allow_utime = -1;
> > + sbi->options.iocharset = exfat_default_iocharset;
> > + sbi->options.errors = EXFAT_ERRORS_RO;
> > + }
> >
> > fc->s_fs_info = sbi;
> > fc->ops = &exfat_context_ops;
> > --
> > 2.17.1
>
>
> Hi Namjae,
>
> 2020년 6월 15일 (월) 오전 9:14, Namjae Jeon <[email protected]>님이 작성:
> >
> > Hi Hyunchul,
> > > We need to commit dirty metadata and pages to disk before remounting exfat as read-only.
> > >
> > > This fixes a failure in xfstests generic/452
> > Could you please elaborate more the reason why generic/452 in xfstests failed ?
>
> xfstests generic/452 does the following.
> cp /bin/ls <exfat>/
> mount -o remount,ro <exfat>
>
> the <exfat>/ls file is corrupted, because while exfat is remounted as read-only, exfat doesn't have a
> chance to commit metadata and vfs invalidates page caches in a block device.
Got it.
>
> I will put this explanation in a commit message.
Good.
>
> > >
> > > Signed-off-by: Hyunchul Lee <[email protected]>
> > > ---
> > > fs/exfat/super.c | 19 +++++++++++++++++++
> > > 1 file changed, 19 insertions(+)
> > >
> > > diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> > > e650e65536f8..61c6cf240c19 100644
> > > --- a/fs/exfat/super.c
> > > +++ b/fs/exfat/super.c
> > > @@ -693,10 +693,29 @@ static void exfat_free(struct fs_context *fc)
> > > }
> > > }
> > >
> > > +static int exfat_reconfigure(struct fs_context *fc) {
> > > + struct super_block *sb = fc->root->d_sb;
> > > + int ret;
> > int ret = 0;
> > > + bool new_rdonly;
> > > +
> > > + new_rdonly = fc->sb_flags & SB_RDONLY;
> > > + if (new_rdonly != sb_rdonly(sb)) {
> > If you modify it like this, would not we need new_rdonly?
> > if (fc->sb_flags & SB_RDONLY && !sb_rdonly(sb))
> >
> This condition means that mount options are changed from "rw" to "ro", or "ro" to "rw".
>
> > > + if (new_rdonly) {
>
> And this condition means these options are changed from "rw" to "ro".
> It seems better to change two conditions to the one you suggested, or remove those. because
> sync_filesystem returns 0 when the filesystem is mounted as read-only.
The latter would be fine.
>
> > > + /* volume flag will be updated in exfat_sync_fs */
> > > + ret = sync_filesystem(sb);
> > > + if (ret < 0)
> > > + return ret;
> > I think that this ret check can be removed by using return ret; below ?
>
> Okay, I will apply this.
> Thank you for your comments!
Thanks for your patch!
>
>
> > > + }
> > > + }
> > > + return 0;
> > return ret;
> > > +}
> > > +
> > > static const struct fs_context_operations exfat_context_ops = {
> > > .parse_param = exfat_parse_param,
> > > .get_tree = exfat_get_tree,
> > > .free = exfat_free,
> > > + .reconfigure = exfat_reconfigure,
> > > };
> > >
> > > static int exfat_init_fs_context(struct fs_context *fc)
> > > --
> > > 2.17.1
> >
> >