| Message ID | 1449468402-27914-1-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Monday 07 Dec 2015 14:06:42 Qu Wenruo wrote: > Introduce a new mount option "nologreplay" to co-operate with "ro" mount > option to get real readonly mount, like "norecovery" in ext* and xfs. > > Since the new parse_options() need to check new flags at remount time, > so add a new parameter for parse_options(). > Hello Qu, The patch should add an entry into btrfs_show_options() for 'mount' command to be able to display an entry corresponding to 'nologreplay' mount option. Even after adding such an entry to btrfs_show_options(), the following occurs, # mount -o nologreplay,ro /dev/loop6 /mnt/ # mount | grep loop6 /dev/loop6 on /mnt type btrfs (ro,relatime,seclabel,nologreplay,space_cache,subvolid=5,subvol=/) # mount -o remount,rw /dev/loop6 /mnt/ # mount | grep loop6 /dev/loop6 on /mnt type btrfs (rw,relatime,seclabel,nologreplay,space_cache,subvolid=5,subvol=/) As can be seen from the last line, both 'rw' and 'nologreplay' options are set.
On 12/7/15 12:06 AM, Qu Wenruo wrote: > Introduce a new mount option "nologreplay" to co-operate with "ro" mount > option to get real readonly mount, like "norecovery" in ext* and xfs. > > Since the new parse_options() need to check new flags at remount time, > so add a new parameter for parse_options(). > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > Documentation/filesystems/btrfs.txt | 5 +++++ > fs/btrfs/ctree.h | 4 +++- > fs/btrfs/disk-io.c | 7 ++++--- > fs/btrfs/super.c | 20 +++++++++++++++++--- > 4 files changed, 29 insertions(+), 7 deletions(-) > > diff --git a/Documentation/filesystems/btrfs.txt b/Documentation/filesystems/btrfs.txt > index c772b47..ac4ed68 100644 > --- a/Documentation/filesystems/btrfs.txt > +++ b/Documentation/filesystems/btrfs.txt > @@ -168,6 +168,11 @@ Options with (*) are default options and will not show in the mount options. > notreelog > Enable/disable the tree logging used for fsync and O_SYNC writes. > > + nologreplay > + Disable the log tree replay at mount time for real read-only mount. > + Must be use with "ro" mount option and can't be disabled by mount > + option. This documentation is not clear to me - "can't be disabled by mount option?" I think you mean to talk about remount here? Perhaps something like: "... Must be used with 'ro' mount option. A filesystem mounted with the 'nologreplay' option cannot transition to a read-write mount via remount,rw - the filesystem must be unmounted and remounted if read-write access is desired." Thanks, -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015-12-07 01:06, Qu Wenruo wrote: > Introduce a new mount option "nologreplay" to co-operate with "ro" mount > option to get real readonly mount, like "norecovery" in ext* and xfs. > > Since the new parse_options() need to check new flags at remount time, > so add a new parameter for parse_options(). > Passes xfstests and a handful of other things that I really should just take the time to integrate into xfstests, so: Tested-by: Austin S. Hemmelgarn <ahferroin7@gmail.com>
On Monday 07 Dec 2015 10:27:05 Eric Sandeen wrote: > On 12/7/15 12:06 AM, Qu Wenruo wrote: > > Introduce a new mount option "nologreplay" to co-operate with "ro" mount > > option to get real readonly mount, like "norecovery" in ext* and xfs. > > > > Since the new parse_options() need to check new flags at remount time, > > so add a new parameter for parse_options(). > > > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > > --- > > > > Documentation/filesystems/btrfs.txt | 5 +++++ > > fs/btrfs/ctree.h | 4 +++- > > fs/btrfs/disk-io.c | 7 ++++--- > > fs/btrfs/super.c | 20 +++++++++++++++++--- > > 4 files changed, 29 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/filesystems/btrfs.txt > > b/Documentation/filesystems/btrfs.txt index c772b47..ac4ed68 100644 > > --- a/Documentation/filesystems/btrfs.txt > > +++ b/Documentation/filesystems/btrfs.txt > > @@ -168,6 +168,11 @@ Options with (*) are default options and will not > > show in the mount options.> > > notreelog > > > > Enable/disable the tree logging used for fsync and O_SYNC writes. > > > > + nologreplay > > + Disable the log tree replay at mount time for real read-only mount. > > + Must be use with "ro" mount option and can't be disabled by mount > > + option. > > This documentation is not clear to me - "can't be disabled by mount option?" > > I think you mean to talk about remount here? Perhaps something like: > > "... Must be used with 'ro' mount option. A filesystem mounted with the > 'nologreplay' option cannot transition to a read-write mount via > remount,rw - the filesystem must be unmounted and remounted if read-write > access is desired." > Eric, I had assumed the same logic with respect to the transition from 'ro' to 'rw' via remount. But when doing so, btrfs_remount() flags an error only when a valid 'tree log' tree is present in the filesystem i.e. btrfs_super_block->log_root has a non-zero value. Otherwise, btrfs_remount() does not seem to have any problem with the transition from 'ro' to 'rw'.
On 12/7/15 10:52 AM, Chandan Rajendra wrote: > On Monday 07 Dec 2015 10:27:05 Eric Sandeen wrote: >> On 12/7/15 12:06 AM, Qu Wenruo wrote: >>> Introduce a new mount option "nologreplay" to co-operate with "ro" mount >>> option to get real readonly mount, like "norecovery" in ext* and xfs. >>> >>> Since the new parse_options() need to check new flags at remount time, >>> so add a new parameter for parse_options(). >>> >>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >>> --- >>> >>> Documentation/filesystems/btrfs.txt | 5 +++++ >>> fs/btrfs/ctree.h | 4 +++- >>> fs/btrfs/disk-io.c | 7 ++++--- >>> fs/btrfs/super.c | 20 +++++++++++++++++--- >>> 4 files changed, 29 insertions(+), 7 deletions(-) >>> >>> diff --git a/Documentation/filesystems/btrfs.txt >>> b/Documentation/filesystems/btrfs.txt index c772b47..ac4ed68 100644 >>> --- a/Documentation/filesystems/btrfs.txt >>> +++ b/Documentation/filesystems/btrfs.txt >>> @@ -168,6 +168,11 @@ Options with (*) are default options and will not >>> show in the mount options.> >>> notreelog >>> >>> Enable/disable the tree logging used for fsync and O_SYNC writes. >>> >>> + nologreplay >>> + Disable the log tree replay at mount time for real read-only mount. >>> + Must be use with "ro" mount option and can't be disabled by mount >>> + option. >> >> This documentation is not clear to me - "can't be disabled by mount option?" >> >> I think you mean to talk about remount here? Perhaps something like: >> >> "... Must be used with 'ro' mount option. A filesystem mounted with the >> 'nologreplay' option cannot transition to a read-write mount via >> remount,rw - the filesystem must be unmounted and remounted if read-write >> access is desired." >> > Eric, I had assumed the same logic with respect to the transition from 'ro' to > 'rw' via remount. But when doing so, btrfs_remount() flags an error only when > a valid 'tree log' tree is present in the filesystem > i.e. btrfs_super_block->log_root has a non-zero value. Otherwise, > btrfs_remount() does not seem to have any problem with the transition from > 'ro' to 'rw'. Ok, I don't know if that's intended - but I think the docs should be clarified to explicitly state the expected behavior in any case. FWIW, new mount options and their descriptions should be added to BTRFS-MOUNT(5) as well. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-12-07 at 11:29 -0600, Eric Sandeen wrote: > FWIW, new mount options and their descriptions should be added to > BTRFS-MOUNT(5) > as well. Also, from the end-user perspective, there should be: 1) another option like (hard-ro) which is defined to imply any other options that are required to make a mount truly read-only (i.e. right now: ro and nologreplay... but in the future any other features that may be required to make read-only really read-only). and/or 2) a section that describes "ro" in btrfs-mount(5) which describes that normal "ro" alone may cause changes on the device and which then refers to hard-ro and/or the list of options (currently nologreplay) which are required right now to make it truly ro. I think this is important as an end-user probably expects "ro" to be truly ro, so if he looks it up in the documentation (2) he should find enough information that this isn't the case and what to do instead. Further, as one might expect that in the future, other places (than just the log) may cause changes to a device, even though mounted ro... I think it's better for the end user to also have a real "hard-ro" like option, than a possibly growing list of "noXYZ" where the end-user may have no clue that something else is now also required to get the truly read-only behaviour. Cheers, Chris.
On 12/7/15 2:54 PM, Christoph Anton Mitterer wrote: ... > 2) a section that describes "ro" in btrfs-mount(5) which describes that > normal "ro" alone may cause changes on the device and which then refers > to hard-ro and/or the list of options (currently nologreplay) which are > required right now to make it truly ro. > > > I think this is important as an end-user probably expects "ro" to be > truly ro, Yeah, I don't know that this is true. It hasn't been true for over a decade (2?), with the most widely-used filesystem in linux history, i.e. ext3. So if btrfs wants to go on this re-education crusade, more power to you, but I don't know that it's really a fight worth fighting. ;) > so if he looks it up in the documentation (2) he should find > enough information that this isn't the case and what to do instead. > Further, as one might expect that in the future, other places (than > just the log) may cause changes to a device, even though mounted ro... > I think it's better for the end user to also have a real "hard-ro" like > option, than a possibly growing list of "noXYZ" where the end-user may > have no clue that something else is now also required to get the truly > read-only behaviour. # blockdev --setro ... -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/07/2015 11:38 PM, Chandan Rajendra wrote: > On Monday 07 Dec 2015 14:06:42 Qu Wenruo wrote: >> Introduce a new mount option "nologreplay" to co-operate with "ro" mount >> option to get real readonly mount, like "norecovery" in ext* and xfs. >> >> Since the new parse_options() need to check new flags at remount time, >> so add a new parameter for parse_options(). >> > > Hello Qu, > The patch should add an entry into btrfs_show_options() for 'mount' command to > be able to display an entry corresponding to 'nologreplay' mount option. Thanks for the advice. I just forgot show_mount_options(). And for option to disable "nologreplay", I follow the behavior of xfs and ext4 "norecovery" behavior. Which means no option to disable "norecovery", as it is only used for debugging case. So the only method is to umount it and mount it again. > > Even after adding such an entry to btrfs_show_options(), > the following occurs, > # mount -o nologreplay,ro /dev/loop6 /mnt/ > # mount | grep loop6 > /dev/loop6 on /mnt type btrfs (ro,relatime,seclabel,nologreplay,space_cache,subvolid=5,subvol=/) > # mount -o remount,rw /dev/loop6 /mnt/ > # mount | grep loop6 > /dev/loop6 on /mnt type btrfs (rw,relatime,seclabel,nologreplay,space_cache,subvolid=5,subvol=/) > > As can be seen from the last line, both 'rw' and 'nologreplay' options are set. > I'll double check it, maybe it bypassed btrfs_parse_options(), and bypassed the check in it. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-12-07 at 17:06 -0600, Eric Sandeen wrote: > Yeah, I don't know that this is true. It hasn't been true for over a > decade (2?), with the most widely-used filesystem in linux history, > i.e. > ext3. Based on what? I'd now many sysadmins who don't expect that e.g. the journal is replayed when ext* is mounted ro. > So if btrfs wants to go on this re-education crusade, more power > to you, but I don't know that it's really a fight worth fighting. ;) I don't think there's a bight fight necessary, is it? Just properly document all options and not only from the developers or expert-users PoV. Using blockdev --setro is of course an alternative to a dedicated "hard-ro" option (or something similar that goes by a better name),... OTOH I don't think that having such an option would cause big problems or "religion wars" ;) Cheers, Chris.
Austin S Hemmelgarn wrote on 2015/12/07 11:36 -0500: > On 2015-12-07 01:06, Qu Wenruo wrote: >> Introduce a new mount option "nologreplay" to co-operate with "ro" mount >> option to get real readonly mount, like "norecovery" in ext* and xfs. >> >> Since the new parse_options() need to check new flags at remount time, >> so add a new parameter for parse_options(). >> > > Passes xfstests and a handful of other things that I really should just > take the time to integrate into xfstests, so: > Tested-by: Austin S. Hemmelgarn <ahferroin7@gmail.com> > Thanks for the test. But I'm afraid you may need to test v2 patch again, as the v2 changed some behavior. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015-12-07 18:06, Eric Sandeen wrote: > On 12/7/15 2:54 PM, Christoph Anton Mitterer wrote: > > ... > >> 2) a section that describes "ro" in btrfs-mount(5) which describes that >> normal "ro" alone may cause changes on the device and which then refers >> to hard-ro and/or the list of options (currently nologreplay) which are >> required right now to make it truly ro. >> >> >> I think this is important as an end-user probably expects "ro" to be >> truly ro, > > Yeah, I don't know that this is true. It hasn't been true for over a > decade (2?), with the most widely-used filesystem in linux history, i.e. > ext3. So if btrfs wants to go on this re-education crusade, more power > to you, but I don't know that it's really a fight worth fighting. ;) > Actually, AFAICT, it's been at least 4.5 decades. Last I checked, this dates back to the original UNIX filesystems, which still updated atimes even when mounted RO. Despite this, it really isn't a widely known or well documented behavior outside of developers, forensic specialists, and people who have had to deal with the implications it has on data recovery. There really isn't any way that the user would know about it without being explicitly told, and it's something that can have a serious impact on being able to recover a broken filesystem. TBH, I really feel that _every_ filesystem's documentation should have something about how to make it mount truly read-only, even if it's just a reference to how to mark the block device read-only.
On 2015-12-08 01:08, Qu Wenruo wrote: > > > Austin S Hemmelgarn wrote on 2015/12/07 11:36 -0500: >> On 2015-12-07 01:06, Qu Wenruo wrote: >>> Introduce a new mount option "nologreplay" to co-operate with "ro" mount >>> option to get real readonly mount, like "norecovery" in ext* and xfs. >>> >>> Since the new parse_options() need to check new flags at remount time, >>> so add a new parameter for parse_options(). >>> >> >> Passes xfstests and a handful of other things that I really should just >> take the time to integrate into xfstests, so: >> Tested-by: Austin S. Hemmelgarn <ahferroin7@gmail.com> >> > Thanks for the test. > > But I'm afraid you may need to test v2 patch again, as the v2 changed > some behavior. > That's OK, I should have results for you some time later today.
On Tue, 2015-12-08 at 07:15 -0500, Austin S Hemmelgarn wrote: > Despite this, it really isn't a widely known or well documented > behavior > outside of developers, forensic specialists, and people who have had > to > deal with the implications it has on data recovery. There really > isn't > any way that the user would know about it without being explicitly > told, > and it's something that can have a serious impact on being able to > recover a broken filesystem. TBH, I really feel that _every_ > filesystem's documentation should have something about how to make it > mount truly read-only, even if it's just a reference to how to mark > the > block device read-only. Exactly what I've meant. And the developers here, should definitely consider that every normal end-user, may easily assume the role of e.g. a forensics specialist (especially with btrfs ;-) ), when recovery in case of corruptions is tried. I don't think that "it has always been improperly documented" (i.e. the "ro" option) is a good excuse to continue doing it that way =) Cheers, Chris.
On 2015-12-08 14:20, Christoph Anton Mitterer wrote: > On Tue, 2015-12-08 at 07:15 -0500, Austin S Hemmelgarn wrote: >> Despite this, it really isn't a widely known or well documented >> behavior >> outside of developers, forensic specialists, and people who have had >> to >> deal with the implications it has on data recovery. There really >> isn't >> any way that the user would know about it without being explicitly >> told, >> and it's something that can have a serious impact on being able to >> recover a broken filesystem. TBH, I really feel that _every_ >> filesystem's documentation should have something about how to make it >> mount truly read-only, even if it's just a reference to how to mark >> the >> block device read-only. > Exactly what I've meant. > > And the developers here, should definitely consider that every normal > end-user, may easily assume the role of e.g. a forensics specialist > (especially with btrfs ;-) ), when recovery in case of corruptions is > tried. > > > I don't think that "it has always been improperly documented" (i.e. the > "ro" option) is a good excuse to continue doing it that way =) Agreed, 'but it's always been that way' is never a valid argument, and the fact that people who have been working on UNIX for decades know it doesn't mean that it's something that people will just inherently know. The only reason it was that way to begin with is because it was assumed that everyone dealing with computers had a huge amount of domain specific knowledge of them (this was a valid assumption back in 1970, it hasn't been a valid assumption since at least 1990). Stuff that seems obvious to people who have been working on it for years isn't necessarily obvious to people who have limited experience with it (I recently had to explain to a friend who had almost no networking background how IP addresses are just an abstraction for MAC addresses, and how it's not possible to block WiFi access based on an IP address; it took me three tries and eventually making the analogy of street addresses being an abstraction for geographical coordinates before he finally got it). TBH, the only reason I knew about this rather annoying detail of filesystem implementation before using BTRFS is because of dealing with shared storage on VM's (that was an interesting week of debugging and restoring backups before I finally figured out what was going on).
diff --git a/Documentation/filesystems/btrfs.txt b/Documentation/filesystems/btrfs.txt index c772b47..ac4ed68 100644 --- a/Documentation/filesystems/btrfs.txt +++ b/Documentation/filesystems/btrfs.txt @@ -168,6 +168,11 @@ Options with (*) are default options and will not show in the mount options. notreelog Enable/disable the tree logging used for fsync and O_SYNC writes. + nologreplay + Disable the log tree replay at mount time for real read-only mount. + Must be use with "ro" mount option and can't be disabled by mount + option. + recovery Enable autorecovery attempts if a bad tree root is found at mount time. Currently this scans a list of several previous tree roots and tries to diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index a0165c6..c54ff25 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -2184,6 +2184,7 @@ struct btrfs_ioctl_defrag_range_args { #define BTRFS_MOUNT_RESCAN_UUID_TREE (1 << 23) #define BTRFS_MOUNT_FRAGMENT_DATA (1 << 24) #define BTRFS_MOUNT_FRAGMENT_METADATA (1 << 25) +#define BTRFS_MOUNT_NOLOGREPLAY (1 << 26) #define BTRFS_DEFAULT_COMMIT_INTERVAL (30) #define BTRFS_DEFAULT_MAX_INLINE (8192) @@ -4070,7 +4071,8 @@ void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info); ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size); /* super.c */ -int btrfs_parse_options(struct btrfs_root *root, char *options); +int btrfs_parse_options(struct btrfs_root *root, char *options, + unsigned long new_flags); int btrfs_sync_fs(struct super_block *sb, int wait); #ifdef CONFIG_PRINTK diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 1eb0839..617bf4f 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2711,7 +2711,7 @@ int open_ctree(struct super_block *sb, */ fs_info->compress_type = BTRFS_COMPRESS_ZLIB; - ret = btrfs_parse_options(tree_root, options); + ret = btrfs_parse_options(tree_root, options, sb->s_flags); if (ret) { err = ret; goto fail_alloc; @@ -3009,8 +3009,9 @@ retry_root_backup: if (ret) goto fail_trans_kthread; - /* do not make disk changes in broken FS */ - if (btrfs_super_log_root(disk_super) != 0) { + /* do not make disk changes in broken FS or nologreplay is given */ + if (btrfs_super_log_root(disk_super) != 0 && + !btrfs_test_opt(tree_root, NOLOGREPLAY)) { ret = btrfs_replay_log(fs_info, fs_devices); if (ret) { err = ret; diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 24154e4..3b1dcc1 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -302,7 +302,7 @@ enum { Opt_check_integrity_print_mask, Opt_fatal_errors, Opt_rescan_uuid_tree, Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard, Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow, - Opt_datasum, Opt_treelog, Opt_noinode_cache, + Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_nologreplay, #ifdef CONFIG_BTRFS_DEBUG Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all, #endif @@ -334,6 +334,7 @@ static match_table_t tokens = { {Opt_noacl, "noacl"}, {Opt_notreelog, "notreelog"}, {Opt_treelog, "treelog"}, + {Opt_nologreplay, "nologreplay"}, {Opt_flushoncommit, "flushoncommit"}, {Opt_noflushoncommit, "noflushoncommit"}, {Opt_ratio, "metadata_ratio=%d"}, @@ -371,7 +372,8 @@ static match_table_t tokens = { * reading in a new superblock is parsed here. * XXX JDM: This needs to be cleaned up for remount. */ -int btrfs_parse_options(struct btrfs_root *root, char *options) +int btrfs_parse_options(struct btrfs_root *root, char *options, + unsigned long new_flags) { struct btrfs_fs_info *info = root->fs_info; substring_t args[MAX_OPT_ARGS]; @@ -587,6 +589,10 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) btrfs_clear_and_info(root, NOTREELOG, "enabling tree log"); break; + case Opt_nologreplay: + btrfs_set_and_info(root, NOLOGREPLAY, + "disabling log replay at mount time"); + break; case Opt_flushoncommit: btrfs_set_and_info(root, FLUSHONCOMMIT, "turning on flush-on-commit"); @@ -753,6 +759,14 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) break; } } + /* + * Extra check for current option against current flag + */ + if (btrfs_test_opt(root, NOLOGREPLAY) && !(new_flags & MS_RDONLY)) { + btrfs_err(root->fs_info, + "nologreplay must be used with ro mount option"); + ret = -EINVAL; + } out: if (!ret && btrfs_test_opt(root, SPACE_CACHE)) btrfs_info(root->fs_info, "disk space caching is enabled"); @@ -1637,7 +1651,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) } } - ret = btrfs_parse_options(root, data); + ret = btrfs_parse_options(root, data, *flags); if (ret) { ret = -EINVAL; goto restore;
Introduce a new mount option "nologreplay" to co-operate with "ro" mount option to get real readonly mount, like "norecovery" in ext* and xfs. Since the new parse_options() need to check new flags at remount time, so add a new parameter for parse_options(). Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- Documentation/filesystems/btrfs.txt | 5 +++++ fs/btrfs/ctree.h | 4 +++- fs/btrfs/disk-io.c | 7 ++++--- fs/btrfs/super.c | 20 +++++++++++++++++--- 4 files changed, 29 insertions(+), 7 deletions(-)