Re: [PATCH] Btrfs: disallow 'btrfs {balance,replace} cancel' on ro mounts

[Stefan Behrens <sbehrens@giantdisaster.de>]

On Fri, 11 Oct 2013 09:13:24 +0800, Wang Shilong wrote:
> On 10/11/2013 01:40 AM, Ilya Dryomov wrote:
> 
> I have a question in my mind.
> 
> Can we reach a state that there is operation in progress when filesystem
> has been readonly?If we do cancel operations on a ro filesystem, we should
> get "No operations in progress" .

Well, it's arguable what ro means. No write to the devices at all? Or
replay log on mount (which means to write to the filesystem), but no
write access in addition to that? Or allow filesystem internal things to
be modified and written to disk, like it was done when the balance or
replace control items were modified on disk when the cancelling was
requested?

In any case, don't make it more complicated then necessary IMO, and
return EROFS if someone calls cancel on a ro filesystem regardless of
the state of the operation. It only adds errors to try to distuingish
such things and is of no benefit for anybody IMHO.


>> For both balance and replace, cancelling involves changing the on-disk
>> state and committing a transaction, which is not a good thing to do on
>> read-only filesystems.
>>
>> Cc: Stefan Behrens <sbehrens@giantdisaster.de>
>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
>> ---
>>   fs/btrfs/dev-replace.c |    3 +++
>>   fs/btrfs/volumes.c     |    3 +++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index 9efb94e..98df261 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -650,6 +650,9 @@ static u64 __btrfs_dev_replace_cancel(struct
>> btrfs_fs_info *fs_info)
>>       u64 result;
>>       int ret;
>>   +    if (fs_info->sb->s_flags & MS_RDONLY)
>> +        return -EROFS;
>> +
>>       mutex_lock(&dev_replace->lock_finishing_cancel_unmount);
>>       btrfs_dev_replace_lock(dev_replace);
>>       switch (dev_replace->replace_state) {
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index a306db9..2630f38 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -3424,6 +3424,9 @@ int btrfs_pause_balance(struct btrfs_fs_info
>> *fs_info)
>>     int btrfs_cancel_balance(struct btrfs_fs_info *fs_info)
>>   {
>> +    if (fs_info->sb->s_flags & MS_RDONLY)
>> +        return -EROFS;
>> +
>>       mutex_lock(&fs_info->balance_mutex);
>>       if (!fs_info->balance_ctl) {
>>           mutex_unlock(&fs_info->balance_mutex);
>