On Fri, 23 Aug 2013 14:54:50 +0800, Wang Shilong wrote: > Hey Stefan, > > On 08/20/2013 12:51 AM, Stefan Behrens wrote: >> If you start the replace procedure on a read only filesystem, at >> the end the procedure fails to write the updated dev_items to the >> chunk tree. The problem is that this error is not indicated except >> for a WARN_ON(). If the user now thinks that everything was done >> as expected and destroys the source device (with mkfs or with a >> hammer). The next mount fails with "failed to read chunk root" and >> the filesystem is gone. >> >> This commit adds code to fail the attempt to start the replace >> procedure if the filesystem is mounted read-only. >> >> Signed-off-by: Stefan Behrens <[email protected]> >> Cc: <[email protected]> # 3.10+ >> --- >> fs/btrfs/ioctl.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 3e36626..bf42d41 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -3653,6 +3653,9 @@ static long btrfs_ioctl_dev_replace(struct btrfs_root >> *root, void __user *arg) >> >> switch (p->cmd) { >> case BTRFS_IOCTL_DEV_REPLACE_CMD_START: >> + if (root->fs_info->sb->s_flags & MS_RDONLY) >> + return -EROFS; >> + > > This is not really safe. Consideringļ¼ > > Task 1 Task2 > |--->sb->s_flags & MS_RDONLY > > Remount filesyste RO > > |--->do replace operations > > For the above case, we will continue to do device replace while the > filesystem is READONLY. > and i think mnt_want_file() will be a right choice.
You are right that a window for a race condition remains and that this is therefore not a correct solution. The problem that I have with surrounding the long running replace procedure with mnt_want_write_file/mnt_drop_write_file is that I am unable to remount read-only during this time. remount read-only usually suspends the replace operation, but with mnt_want_write_file it fails and the Btrfs remount code is not called. This would be a problem if some (non-Btrfs-replace-aware) software tries to remount the filesystem read-only. Therefore I still think that the quick & dirty read-only check that I added is the better solution. This happens when mnt_want_write_file() is used: # btrfs replace start /dev/sdi /dev/sde /mnt2 # mount -o remount,ro /dev/sdi /mnt2 mount: you must specify the filesystem type # echo $? 32 # btrfs replace cancel /mnt2 # mount -o remount,ro /dev/sdi /mnt2 # echo $? 0 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
