* [PATCH 1/2] exfat: call sync_filesystem for read-only remount @ 2020-06-12 9:42 ` Hyunchul Lee 2020-06-12 9:42 ` [PATCH 2/2] exfat: allow to change some mount options for remount Hyunchul Lee 2020-06-15 0:14 ` [PATCH 1/2] exfat: call sync_filesystem for read-only remount Namjae Jeon 0 siblings, 2 replies; 7+ messages in thread From: Hyunchul Lee @ 2020-06-12 9:42 UTC (permalink / raw) To: Namjae Jeon, Sungjong Seo; +Cc: linux-fsdevel, linux-kernel, kernel-team 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 <hyc.lee@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] exfat: allow to change some mount options for remount 2020-06-12 9:42 ` [PATCH 1/2] exfat: call sync_filesystem for read-only remount Hyunchul Lee @ 2020-06-12 9:42 ` Hyunchul Lee 2020-06-15 0:18 ` Namjae Jeon 2020-06-15 0:14 ` [PATCH 1/2] exfat: call sync_filesystem for read-only remount Namjae Jeon 1 sibling, 1 reply; 7+ messages in thread From: Hyunchul Lee @ 2020-06-12 9:42 UTC (permalink / raw) To: Namjae Jeon, Sungjong Seo; +Cc: linux-fsdevel, linux-kernel, kernel-team Allow to change permission masks, allow_utime, errors. But ignore other options. Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH 2/2] exfat: allow to change some mount options for remount 2020-06-12 9:42 ` [PATCH 2/2] exfat: allow to change some mount options for remount Hyunchul Lee @ 2020-06-15 0:18 ` Namjae Jeon 2020-06-15 2:12 ` Hyunchul Lee 0 siblings, 1 reply; 7+ messages in thread From: Namjae Jeon @ 2020-06-15 0:18 UTC (permalink / raw) To: 'Hyunchul Lee' Cc: linux-fsdevel, linux-kernel, 'Sungjong Seo' > Allow to change permission masks, allow_utime, errors. But ignore other options. > > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] exfat: allow to change some mount options for remount 2020-06-15 0:18 ` Namjae Jeon @ 2020-06-15 2:12 ` Hyunchul Lee 0 siblings, 0 replies; 7+ messages in thread From: Hyunchul Lee @ 2020-06-15 2:12 UTC (permalink / raw) To: Namjae Jeon; +Cc: linux-fsdevel, LKML, Sungjong Seo 2020년 6월 15일 (월) 오전 9:18, Namjae Jeon <namjae.jeon@samsung.com>님이 작성: > > > Allow to change permission masks, allow_utime, errors. But ignore other options. > > > > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/2] exfat: call sync_filesystem for read-only remount 2020-06-12 9:42 ` [PATCH 1/2] exfat: call sync_filesystem for read-only remount Hyunchul Lee 2020-06-12 9:42 ` [PATCH 2/2] exfat: allow to change some mount options for remount Hyunchul Lee @ 2020-06-15 0:14 ` Namjae Jeon 2020-06-15 2:10 ` Hyunchul Lee 1 sibling, 1 reply; 7+ messages in thread From: Namjae Jeon @ 2020-06-15 0:14 UTC (permalink / raw) To: 'Hyunchul Lee' Cc: linux-fsdevel, linux-kernel, 'Sungjong Seo' 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 <hyc.lee@gmail.com> > --- > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] exfat: call sync_filesystem for read-only remount 2020-06-15 0:14 ` [PATCH 1/2] exfat: call sync_filesystem for read-only remount Namjae Jeon @ 2020-06-15 2:10 ` Hyunchul Lee 2020-06-15 2:56 ` Namjae Jeon 0 siblings, 1 reply; 7+ messages in thread From: Hyunchul Lee @ 2020-06-15 2:10 UTC (permalink / raw) To: Namjae Jeon; +Cc: linux-fsdevel, LKML, Sungjong Seo Hi Namjae, 2020년 6월 15일 (월) 오전 9:14, Namjae Jeon <namjae.jeon@samsung.com>님이 작성: > > 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 <hyc.lee@gmail.com> > > --- > > 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 1/2] exfat: call sync_filesystem for read-only remount 2020-06-15 2:10 ` Hyunchul Lee @ 2020-06-15 2:56 ` Namjae Jeon 0 siblings, 0 replies; 7+ messages in thread From: Namjae Jeon @ 2020-06-15 2:56 UTC (permalink / raw) To: 'Hyunchul Lee' Cc: linux-fsdevel, 'LKML', 'Sungjong Seo' > Hi Namjae, > > 2020년 6월 15일 (월) 오전 9:14, Namjae Jeon <namjae.jeon@samsung.com>님이 작성: > > > > 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 <hyc.lee@gmail.com> > > > --- > > > 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 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-15 2:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20200612094312epcas1p1d8be51e8ab6e26b622e3c8437a20cfcf@epcas1p1.samsung.com> 2020-06-12 9:42 ` [PATCH 1/2] exfat: call sync_filesystem for read-only remount Hyunchul Lee 2020-06-12 9:42 ` [PATCH 2/2] exfat: allow to change some mount options for remount Hyunchul Lee 2020-06-15 0:18 ` Namjae Jeon 2020-06-15 2:12 ` Hyunchul Lee 2020-06-15 0:14 ` [PATCH 1/2] exfat: call sync_filesystem for read-only remount Namjae Jeon 2020-06-15 2:10 ` Hyunchul Lee 2020-06-15 2:56 ` Namjae Jeon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).