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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
