Re: [PATCH 1/2] f2fs: add remount_fs callback support

[Namjae Jeon]

2013/6/4 Gu Zheng <guz.fnst@xxxxxxxxxxxxxx>:
> On 06/01/2013 03:20 PM, Namjae Jeon wrote:
>
>> From: Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
>>
>> Add the f2fs_remount function call which will be used
>> during the filesystem remounting. This function
>> will help us to change the mount options specific to
>> f2fs.
>>
>> Also modify the f2fs background_gc mount option, which
>> will allow the user to dynamically trun on/off the
>> garbage collection in f2fs based on the background_gc
>> value. If background_gc=0, Garbage collection will
>> be turned off & if background_gc=1, Garbage collection
>> will be truned on.
>
>
> Hi Namjae,
Hi. Gu.

>   I think splitting these two changes into single ones seems better.
> Refer to the inline comments.
I don't think so. Mount option background_gc is changed to make
remount_fs working in the correct way.

>
> Thanks,
> Gu
>
>
> Though simply option show is enough, but I think the "background_gc=on/off" is more friendly.
Yes, Agree. I will update.

>
>> +
>> +     /**
>> +      * We stop the GC thread if FS is mounted as RO
>> +      * or if background_gc = 0 is passed in mount
>> +      * option. Also sync the filesystem.
>> +      */
>> +     if ((*flags & MS_RDONLY) || !test_opt(sbi, BG_GC)) {
>
>
> Another condition: The old mount is not RO.
I don't think that it is needed. I think current condition check can
be covered about all cases.
Am I missing something ?

>
>> +             stop_gc_thread(sbi);
>> +             f2fs_sync_fs(sb, 1);
>> +     } else if (test_opt(sbi, BG_GC) && !sbi->gc_thread) {
>> +             err = start_gc_thread(sbi);
>> +             if (err)
>> +                     goto restore_opts;
>> +     }
>> +
>> +     /* Update the POSIXACL Flag */
>> +      sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
>> +             (test_opt(sbi, POSIX_ACL) ? MS_POSIXACL : 0);
>
>
> Maybe you forget to update the flags with MS_RDONLY or ~MS_RDONLY, if the flags changed.
No, we don't need to check this flags. sb-s_flags will be updated by
MS_REMOUNT of vfs.(do_remount_sb)

>
>> +     return 0;
>> +
>> +restore_opts:
>> +     sb->s_flags = old_sb_flags;
>
>
> There is no need to restore sb->s_flags, parse_options() did not change it.
> no need to store the old sb->s_flags too.
Yes, right, I will update.

>
>>
>> -     /* After POR, we can run background GC thread */
>> -     err = start_gc_thread(sbi);
>> -     if (err)
>> -             goto fail;
>> +     /* After POR, we can run background GC thread.*/
>> +     if (!(sb->s_flags & MS_RDONLY)) {
>> +             /**
>> +              * If filesystem is mounted as read-only then
>> +              * do not start the gc_thread.
>> +              */
>
> It seems that the comment here is against with the logic.
hum.. Okay, I will update comment to avoid some confusion.

Thanks for review :)
I will post v2 patch including your opinion soon.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/