lkml.org 
[lkml]   [2021]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [f2fs-dev] [PATCH] Revert "f2fs: give a warning only for readonly partition"
    From
    Date
    On 2021/3/25 9:59, Chao Yu wrote:
    > On 2021/3/25 6:44, Jaegeuk Kim wrote:
    >> On 03/24, Chao Yu wrote:
    >>> On 2021/3/24 12:22, Jaegeuk Kim wrote:
    >>>> On 03/24, Chao Yu wrote:
    >>>>> On 2021/3/24 2:39, Jaegeuk Kim wrote:
    >>>>>> On 03/23, Chao Yu wrote:
    >>>>>>> This reverts commit 938a184265d75ea474f1c6fe1da96a5196163789.
    >>>>>>>
    >>>>>>> Because that commit fails generic/050 testcase which expect failure
    >>>>>>> during mount a recoverable readonly partition.
    >>>>>>
    >>>>>> I think we need to change generic/050, since f2fs can recover this partition,
    >>>>>
    >>>>> Well, not sure we can change that testcase, since it restricts all generic
    >>>>> filesystems behavior. At least, ext4's behavior makes sense to me:
    >>>>>
    >>>>> journal_dev_ro = bdev_read_only(journal->j_dev);
    >>>>> really_read_only = bdev_read_only(sb->s_bdev) | journal_dev_ro;
    >>>>>
    >>>>> if (journal_dev_ro && !sb_rdonly(sb)) {
    >>>>> ext4_msg(sb, KERN_ERR,
    >>>>> "journal device read-only, try mounting with '-o ro'");
    >>>>> err = -EROFS;
    >>>>> goto err_out;
    >>>>> }
    >>>>>
    >>>>> if (ext4_has_feature_journal_needs_recovery(sb)) {
    >>>>> if (sb_rdonly(sb)) {
    >>>>> ext4_msg(sb, KERN_INFO, "INFO: recovery "
    >>>>> "required on readonly filesystem");
    >>>>> if (really_read_only) {
    >>>>> ext4_msg(sb, KERN_ERR, "write access "
    >>>>> "unavailable, cannot proceed "
    >>>>> "(try mounting with noload)");
    >>>>> err = -EROFS;
    >>>>> goto err_out;
    >>>>> }
    >>>>> ext4_msg(sb, KERN_INFO, "write access will "
    >>>>> "be enabled during recovery");
    >>>>> }
    >>>>> }
    >>>>>
    >>>>>> even though using it as readonly. And, valid checkpoint can allow for user to
    >>>>>> read all the data without problem.
    >>>>>
    >>>>>>> if (f2fs_hw_is_readonly(sbi)) {
    >>>>>
    >>>>> Since device is readonly now, all write to the device will fail, checkpoint can
    >>>>> not persist recovered data, after page cache is expired, user can see stale data.
    >>>>
    >>>> My point is, after mount with ro, there'll be no data write which preserves the
    >>>> current status. So, in the next time, we can recover fsync'ed data later, if
    >>>> user succeeds to mount as rw. Another point is, with the current checkpoint, we
    >>>> should not have any corrupted metadata. So, why not giving a chance to show what
    >>>> data remained to user? I think this can be doable only with CoW filesystems.
    >>>
    >>> I guess we're talking about the different things...
    >>>
    >>> Let me declare two different readonly status:
    >>>
    >>> 1. filesystem readonly: file system is mount with ro mount option, and
    >>> app from userspace can not modify any thing of filesystem, but filesystem
    >>> itself can modify data on device since device may be writable.
    >>>
    >>> 2. device readonly: device is set to readonly status via 'blockdev --setro'
    >>> command, and then filesystem should never issue any write IO to the device.
    >>>
    >>> So, what I mean is, *when device is readonly*, rather than f2fs mountpoint
    >>> is readonly (f2fs_hw_is_readonly() returns true as below code, instead of
    >>> f2fs_readonly() returns true), in this condition, we should not issue any
    >>> write IO to device anyway, because, AFAIK, write IO will fail due to
    >>> bio_check_ro() check.
    >>
    >> In that case, mount(2) will try readonly, no?
    >
    > Yes, if device is readonly, mount (2) can not mount/remount device to rw
    > mountpoint.

    Any other concern about this patch?

    Thanks,

    >
    > Thanks,
    >
    >>
    >> # blockdev --setro /dev/vdb
    >> # mount -t f2fs /dev/vdb /mnt/test/
    >> mount: /mnt/test: WARNING: source write-protected, mounted read-only.
    >>
    >>>
    >>> if (f2fs_hw_is_readonly(sbi)) {
    >>> - if (!is_set_ckpt_flags(sbi, CP_UMOUNT_FLAG)) {
    >>> - err = -EROFS;
    >>> + if (!is_set_ckpt_flags(sbi, CP_UMOUNT_FLAG))
    >>> f2fs_err(sbi, "Need to recover fsync data, but write access unavailable");
    >>> - goto free_meta;
    >>> - }
    >>> - f2fs_info(sbi, "write access unavailable, skipping recovery");
    >>> + else
    >>> + f2fs_info(sbi, "write access unavailable, skipping recovery");
    >>> goto reset_checkpoint;
    >>> }
    >>>
    >>> For the case of filesystem is readonly and device is writable, it's fine
    >>> to do recovery in order to let user to see fsynced data.
    >>>
    >>> Thanks,
    >>>
    >>>>
    >>>>>
    >>>>> Am I missing something?
    >>>>>
    >>>>> Thanks,
    >>>>>
    >>>>>>
    >>>>>>>
    >>>>>>> Fixes: 938a184265d7 ("f2fs: give a warning only for readonly partition")
    >>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
    >>>>>>> ---
    >>>>>>> fs/f2fs/super.c | 8 +++++---
    >>>>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
    >>>>>>>
    >>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
    >>>>>>> index b48281642e98..2b78ee11f093 100644
    >>>>>>> --- a/fs/f2fs/super.c
    >>>>>>> +++ b/fs/f2fs/super.c
    >>>>>>> @@ -3952,10 +3952,12 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
    >>>>>>> * previous checkpoint was not done by clean system shutdown.
    >>>>>>> */
    >>>>>>> if (f2fs_hw_is_readonly(sbi)) {
    >>>>>>> - if (!is_set_ckpt_flags(sbi, CP_UMOUNT_FLAG))
    >>>>>>> + if (!is_set_ckpt_flags(sbi, CP_UMOUNT_FLAG)) {
    >>>>>>> + err = -EROFS;
    >>>>>>> f2fs_err(sbi, "Need to recover fsync data, but write access unavailable");
    >>>>>>> - else
    >>>>>>> - f2fs_info(sbi, "write access unavailable, skipping recovery");
    >>>>>>> + goto free_meta;
    >>>>>>> + }
    >>>>>>> + f2fs_info(sbi, "write access unavailable, skipping recovery");
    >>>>>>> goto reset_checkpoint;
    >>>>>>> }
    >>>>>>> --
    >>>>>>> 2.29.2
    >>>>>> .
    >>>>>>
    >>>> .
    >>>>
    >> .
    >>
    >
    >
    > _______________________________________________
    > Linux-f2fs-devel mailing list
    > Linux-f2fs-devel@lists.sourceforge.net
    > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
    > .
    >

    \
     
     \ /
      Last update: 2021-03-26 02:09    [W:2.345 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site