linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Chao Yu <chao@kernel.org>
To: Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <yuchao0@huawei.com>
Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix quota info to adjust recovered data
Date: Mon, 1 Oct 2018 08:35:09 +0800	[thread overview]
Message-ID: <227e112e-a462-6803-5c41-dad47569a55b@kernel.org> (raw)
In-Reply-To: <20180930235827.GB17407@jaegeuk-macbookpro.roam.corp.google.com>

On 2018-10-1 7:58, Jaegeuk Kim wrote:
> On 09/29, Chao Yu wrote:
>> On 2018/9/29 7:40, Jaegeuk Kim wrote:
>>> Testing other fix.
>>>
>>> ---
>>>  fs/f2fs/checkpoint.c |  7 +++++++
>>>  fs/f2fs/f2fs.h       |  1 +
>>>  fs/f2fs/gc.c         | 10 +++++++++-
>>>  fs/f2fs/super.c      | 22 +++++++++++++++++++++-
>>>  4 files changed, 38 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 524b87667cf4..3fde91f41a91 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1494,6 +1494,7 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  {
>>>  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
>>>  	unsigned long long ckpt_ver;
>>> +	bool need_up = false;
>>>  	int err = 0;
>>>  
>>>  	if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
>>> @@ -1506,6 +1507,10 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  		f2fs_msg(sbi->sb, KERN_WARNING,
>>>  				"Start checkpoint disabled!");
>>>  	}
>>> +	if (!is_sbi_flag_set(sbi, SBI_QUOTA_INIT)) {
>>> +		need_up = true;
>>> +		down_read(&sbi->sb->s_umount);
>>
>> This is to avoid show warning when calling dquot_writeback_dquots() in
>> f2fs_quota_sync(), right?
> 
> Yup. Unfortunately, this can't fix all the issues, so I'm testing trylock
> simply in this case.

Oh, that's just warning, it could not be harmful, I think we can simply remove
WARN_ON_ONCE in dquot_writeback_dquots to fix this?

> 
>>
>>> +	}
>>>  	mutex_lock(&sbi->cp_mutex);
>>>  
>>>  	if (!is_sbi_flag_set(sbi, SBI_IS_DIRTY) &&
>>> @@ -1582,6 +1587,8 @@ int f2fs_write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	trace_f2fs_write_checkpoint(sbi->sb, cpc->reason, "finish checkpoint");
>>>  out:
>>>  	mutex_unlock(&sbi->cp_mutex);
>>> +	if (need_up)
>>> +		up_read(&sbi->sb->s_umount);
>>>  	return err;
>>>  }
>>>  
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 57c829dd107e..30194f2f108e 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -1096,6 +1096,7 @@ enum {
>>>  	SBI_IS_SHUTDOWN,			/* shutdown by ioctl */
>>>  	SBI_IS_RECOVERED,			/* recovered orphan/data */
>>>  	SBI_CP_DISABLED,			/* CP was disabled last mount */
>>> +	SBI_QUOTA_INIT,				/* avoid sb->s_umount lock */
>>>  	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
>>>  	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
>>>  	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index adaf5a695b12..deece448cb3b 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -55,9 +55,14 @@ static int gc_thread_func(void *data)
>>>  			f2fs_stop_checkpoint(sbi, false);
>>>  		}
>>>  
>>> -		if (!sb_start_write_trylock(sbi->sb))
>>> +		if (!down_read_trylock(&sbi->sb->s_umount))
>>>  			continue;
>>>  
>>> +		set_sbi_flag(sbi, SBI_QUOTA_INIT);
>>> +
>>> +		if (!sb_start_write_trylock(sbi->sb))
>>> +			goto next_umount;
>>> +
>>>  		/*
>>>  		 * [GC triggering condition]
>>>  		 * 0. GC is not conducted currently.
>>> @@ -104,6 +109,9 @@ static int gc_thread_func(void *data)
>>>  		f2fs_balance_fs_bg(sbi);
>>>  next:
>>>  		sb_end_write(sbi->sb);
>>> +next_umount:
>>> +		clear_sbi_flag(sbi, SBI_QUOTA_INIT);
>>> +		up_read(&sbi->sb->s_umount);
>>>  
>>>  	} while (!kthread_should_stop());
>>>  	return 0;
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index a28c245b1288..40a77a4eb465 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -1029,6 +1029,8 @@ static void f2fs_put_super(struct super_block *sb)
>>>  	int i;
>>>  	bool dropped;
>>>  
>>> +	set_sbi_flag(sbi, SBI_QUOTA_INIT);
>>> +
>>>  	f2fs_quota_off_umount(sb);
>>>  
>>>  	/* prevent remaining shrinker jobs */
>>> @@ -1122,11 +1124,17 @@ int f2fs_sync_fs(struct super_block *sb, int sync)
>>>  
>>>  	if (sync) {
>>>  		struct cp_control cpc;
>>> +		bool keep = is_sbi_flag_set(sbi, SBI_QUOTA_INIT);
>>>  
>>>  		cpc.reason = __get_cp_reason(sbi);
>>>  
>>>  		mutex_lock(&sbi->gc_mutex);
>>> +		if (sbi->sb->s_writers.frozen >= SB_FREEZE_WRITE)
>>> +			set_sbi_flag(sbi, SBI_QUOTA_INIT);
>>> +
>>>  		err = f2fs_write_checkpoint(sbi, &cpc);
>>> +		if (!keep)
>>> +			clear_sbi_flag(sbi, SBI_QUOTA_INIT);
>>>  		mutex_unlock(&sbi->gc_mutex);
>>>  	}
>>>  	f2fs_trace_ios(NULL, 1);
>>> @@ -1534,6 +1542,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
>>>  		}
>>>  	}
>>>  #endif
>>> +	set_sbi_flag(sbi, SBI_QUOTA_INIT);
>>>  
>>>  	/* recover superblocks we couldn't write due to previous RO mount */
>>>  	if (!(*flags & SB_RDONLY) && is_sbi_flag_set(sbi, SBI_NEED_SB_WRITE)) {
>>> @@ -1653,6 +1662,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
>>>  		(test_opt(sbi, POSIX_ACL) ? SB_POSIXACL : 0);
>>>  
>>>  	limit_reserve_root(sbi);
>>> +	clear_sbi_flag(sbi, SBI_QUOTA_INIT);
>>>  	*flags = (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME);
>>>  	return 0;
>>>  restore_gc:
>>> @@ -1673,6 +1683,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
>>>  #endif
>>>  	sbi->mount_opt = org_mount_opt;
>>>  	sb->s_flags = old_sb_flags;
>>> +	clear_sbi_flag(sbi, SBI_QUOTA_INIT);
>>>  	return err;
>>>  }
>>>  
>>> @@ -1706,6 +1717,7 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>>  				congestion_wait(BLK_RW_ASYNC, HZ/50);
>>>  				goto repeat;
>>>  			}
>>> +			set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>  			return PTR_ERR(page);
>>>  		}
>>>  
>>> @@ -1717,6 +1729,7 @@ static ssize_t f2fs_quota_read(struct super_block *sb, int type, char *data,
>>>  		}
>>>  		if (unlikely(!PageUptodate(page))) {
>>>  			f2fs_put_page(page, 1);
>>> +			set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>  			return -EIO;
>>>  		}
>>>  
>>> @@ -1758,6 +1771,7 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
>>>  				congestion_wait(BLK_RW_ASYNC, HZ/50);
>>>  				goto retry;
>>>  			}
>>> +			set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_NEED_REPAIR);
>>>  			break;
>>
>> I added these before, but later I didn't encounter quota corruption w/o
>> them, so I removed them.
>>
>> Do you still hit corruption after this change?
> 
> I found one issue where we must avoid roll-forward recovery, if fsck overwrote some
> blocks to fix quota which will be used for recovery.

That's correct, IMO, the right way is that fsck needs to be aware of whole dnode
chain and block address in dnode block, when doing allocation, it needs to
bypass those block address of dnode and block which can be recovered later by
kernel, so that fsynced data can not be lost.

But, in order to trouble shoot current problem more quickly, we can just disable
kernel recovery once fsck recovers quota file for now.

Thanks,

> 
>>
>> Thanks,
>>
>>>  		}
>>>  
>>> @@ -1794,7 +1808,6 @@ static qsize_t *f2fs_get_reserved_space(struct inode *inode)
>>>  
>>>  static int f2fs_quota_on_mount(struct f2fs_sb_info *sbi, int type)
>>>  {
>>> -
>>>  	if (is_set_ckpt_flags(sbi, CP_QUOTA_NEED_FSCK_FLAG)) {
>>>  		f2fs_msg(sbi->sb, KERN_ERR,
>>>  			"quota sysfile may be corrupted, skip loading it");
>>> @@ -1958,7 +1971,9 @@ static int f2fs_quota_on(struct super_block *sb, int type, int format_id,
>>>  	if (err)
>>>  		return err;
>>>  
>>> +	set_sbi_flag(F2FS_SB(sb), SBI_QUOTA_INIT);
>>>  	err = dquot_quota_on(sb, type, format_id, path);
>>> +	clear_sbi_flag(F2FS_SB(sb), SBI_QUOTA_INIT);
>>>  	if (err)
>>>  		return err;
>>>  
>>> @@ -3179,6 +3194,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>  		goto free_meta_inode;
>>>  	}
>>>  
>>> +	set_sbi_flag(sbi, SBI_QUOTA_INIT);
>>>  	if (__is_set_ckpt_flags(F2FS_CKPT(sbi), CP_QUOTA_NEED_FSCK_FLAG))
>>>  		set_sbi_flag(sbi, SBI_QUOTA_NEED_REPAIR);
>>>  
>>> @@ -3370,6 +3386,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>  				cur_cp_version(F2FS_CKPT(sbi)));
>>>  	f2fs_update_time(sbi, CP_TIME);
>>>  	f2fs_update_time(sbi, REQ_TIME);
>>> +
>>> +	clear_sbi_flag(sbi, SBI_QUOTA_INIT);
>>>  	return 0;
>>>  
>>>  free_meta:
>>> @@ -3434,6 +3452,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>>>  		shrink_dcache_sb(sb);
>>>  		goto try_onemore;
>>>  	}
>>> +	clear_sbi_flag(sbi, SBI_QUOTA_INIT);
>>>  	return err;
>>>  }
>>>  
>>> @@ -3449,6 +3468,7 @@ static void kill_f2fs_super(struct super_block *sb)
>>>  		struct f2fs_sb_info *sbi = F2FS_SB(sb);
>>>  
>>>  		set_sbi_flag(sbi, SBI_IS_CLOSE);
>>> +		set_sbi_flag(sbi, SBI_QUOTA_INIT);
>>>  		f2fs_stop_gc_thread(sbi);
>>>  		f2fs_stop_discard_thread(sbi);
>>>  
>>>
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

  reply	other threads:[~2018-10-01  0:35 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-11 20:15 [PATCH] f2fs: fix quota info to adjust recovered data Jaegeuk Kim
2018-09-11 23:51 ` [f2fs-dev] " Chao Yu
2018-09-12  0:06   ` Jaegeuk Kim
2018-09-12  0:27     ` Jaegeuk Kim
2018-09-12  1:13       ` Chao Yu
2018-09-12  1:25         ` Jaegeuk Kim
2018-09-12  1:40           ` Chao Yu
2018-09-12  1:46             ` Chao Yu
2018-09-12 19:54               ` Jaegeuk Kim
2018-09-12 23:28                 ` Chao Yu
2018-09-18  1:19                   ` Jaegeuk Kim
2018-09-18  1:38                     ` Chao Yu
2018-09-18  2:05                       ` Jaegeuk Kim
2018-09-18 10:13                         ` Chao Yu
2018-09-18 16:45                           ` Jaegeuk Kim
2018-09-19  1:38                             ` Chao Yu
2018-09-19 22:38                               ` Jaegeuk Kim
2018-09-20  9:46                                 ` Chao Yu
2018-09-20 21:42                                   ` Jaegeuk Kim
2018-09-21  7:48                                     ` Chao Yu
2018-09-26  0:29                                       ` Jaegeuk Kim
2018-09-26  1:21                                         ` Chao Yu
2018-09-26  1:44                                           ` Jaegeuk Kim
2018-09-26  2:06                                             ` Chao Yu
2018-09-26  2:09                                               ` Jaegeuk Kim
2018-09-26  2:14                                                 ` Chao Yu
2018-09-27  1:16                                                 ` Chao Yu
2018-09-28 17:37                                                   ` Jaegeuk Kim
2018-09-28 23:40                                                     ` Jaegeuk Kim
2018-09-29 10:38                                                       ` Chao Yu
2018-09-30 23:58                                                         ` Jaegeuk Kim
2018-10-01  0:35                                                           ` Chao Yu [this message]
2018-10-01  1:27                                                             ` Jaegeuk Kim
2018-10-01  1:37                                                               ` Chao Yu
2018-09-12  2:46             ` Jaegeuk Kim
2018-09-12  2:57               ` Chao Yu
2018-09-12 19:50                 ` Jaegeuk Kim
2018-09-12 23:30                   ` Chao Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=227e112e-a462-6803-5c41-dad47569a55b@kernel.org \
    --to=chao@kernel.org \
    --cc=jaegeuk@kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=yuchao0@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).