Re: [PATCH] back-up raw_super in sbi

[He YunLei <heyunlei@huawei.com>]

On 2015/12/9 14:11, Chao Yu wrote:
> Hi,
>
>> -----Original Message-----
>> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
>> Sent: Wednesday, December 09, 2015 2:19 AM
>> To: Yunlei He
>> Cc: linux-f2fs-devel@lists.sourceforge.net; chao2.yu@samsung.com; hebiao6@huawei.com;
>> stuart.li@huawei.com
>> Subject: Re: [f2fs-dev] [PATCH] back-up raw_super in sbi
>>
>> Hi Yunlei,
>>
>> On Tue, Dec 08, 2015 at 09:17:13PM +0800, Yunlei He wrote:
>>> write_checkpoint() tries to get cp_blkaddr from superblock buffer,
>>> if the buffer happen to be destroied by something else, it may
>
> Yunlei,
>
> You mean hacking in memory? could you share more about process of destroying?

I do some test on kernel version 3.10 like this:

with mounted f2fs partition, use dd to erase the first sb

dd if=/dev/zero of=/dev/sdx bs=4k count=1

then sync, and the system will panic.

the log added in function do_checkpoint show:

heyunlei:start_blk = 0

So maybe dd dirty the super block buffer stored in sbi.

But, in latest dev branch, it has no problem, I don't know why.


>
>>> bring in unpredictable effect on f2fs.
>>>
>>> this patch fix it by back-up a raw_super copy.
>>>
>>> Signed-off-by: Yunlei He <heyunlei@huawei.com>
>>> ---
>>>   fs/f2fs/checkpoint.c |  3 ++-
>>>   fs/f2fs/f2fs.h       | 10 ++++++++++
>>>   fs/f2fs/node.c       |  2 +-
>>>   fs/f2fs/super.c      | 12 ++++++++----
>>>   4 files changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index f661d80..1e342fd 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -276,13 +276,14 @@ long sync_meta_pages(struct f2fs_sb_info *sbi, enum page_type type,
>>>   						long nr_to_write)
>>>   {
>>>   	struct address_space *mapping = META_MAPPING(sbi);
>>> -	pgoff_t index = 0, end = LONG_MAX, prev = LONG_MAX;
>>> +	pgoff_t index, end = LONG_MAX, prev = LONG_MAX;
>>>   	struct pagevec pvec;
>>>   	long nwritten = 0;
>>>   	struct writeback_control wbc = {
>>>   		.for_reclaim = 0,
>>>   	};
>>>
>>> +	index = __start_cp_addr(sbi);
>>
>> No need to do this.
>>
>>>   	pagevec_init(&pvec, 0);
>>>
>>>   	while (index <= end) {
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 0052ae8..6afb56c 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -1167,6 +1167,16 @@ static inline block_t __start_sum_addr(struct f2fs_sb_info *sbi)
>>>   	return le32_to_cpu(F2FS_CKPT(sbi)->cp_pack_start_sum);
>>>   }
>>>
>>> +static inline block_t __start_main_addr(struct f2fs_sb_info *sbi)
>>> +{
>>> +	return le32_to_cpu(F2FS_RAW_SUPER(sbi)->main_blkaddr);
>>> +}
>>> +
>>> +static inline block_t __start_sum_addr(struct f2fs_sb_info *sbi)
>>> +{
>>> +	return le32_to_cpu(F2FS_CKPT(sbi)->cp_pack_start_sum);
>>> +}
>>> +
>>
>> Ditto.
>>
>>>   static inline bool inc_valid_node_count(struct f2fs_sb_info *sbi,
>>>   						struct inode *inode)
>>>   {
>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>>> index 7bcbc6e..7d2b0bb 100644
>>> --- a/fs/f2fs/node.c
>>> +++ b/fs/f2fs/node.c
>>> @@ -1161,7 +1161,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
>>>   	pagevec_init(&pvec, 0);
>>>
>>>   next_step:
>>> -	index = 0;
>>> +	index = __start_main_addr(sbi);
>>
>> No, its index is for nid.
>>
>>>   	end = LONG_MAX;
>>>
>>>   	while (index <= end) {
>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>>> index bd7e9c6..d394711 100644
>>> --- a/fs/f2fs/super.c
>>> +++ b/fs/f2fs/super.c
>>> @@ -1040,6 +1040,9 @@ static int read_raw_super_block(struct super_block *sb,
>>>   	struct f2fs_super_block *super;
>>>   	int err = 0;
>>>
>>> +	super = kzalloc(sizeof(struct f2fs_super_block), GFP_KERNEL);
>>> +	if (!super)
>>> +		return -ENOMEM;
>>>   retry:
>>>   	buffer = sb_bread(sb, block);
>>>   	if (!buffer) {
>>> @@ -1055,9 +1058,8 @@ retry:
>>>   		}
>>>   	}
>>>
>>> -	super = (struct f2fs_super_block *)
>>> -		((char *)(buffer)->b_data + F2FS_SUPER_OFFSET);
>>> -
>>> +	memcpy(super, buffer->b_data + F2FS_SUPER_OFFSET,
>>> +					sizeof(struct f2fs_super_block));
>>>   	/* sanity checking of raw super */
>>>   	if (sanity_check_raw_super(sb, super)) {
>>>   		brelse(buffer);
>>> @@ -1090,8 +1092,10 @@ retry:
>>>
>>>   out:
>>>   	/* No valid superblock */
>>> -	if (!*raw_super)
>>> +	if (!*raw_super) {
>>> +		kfree(super);
>>>   		return err;
>>> +	}
>>
>> Need to consider f2fs_commit_super and kfree() in put_super.
>
> How about releasing grabbed block buffer 'sbi->raw_super_buf' since we
> already had one copy of it?
>
> Thanks,
>
>>
>> Could you check out this patch?
>>
>> ---
>>   fs/f2fs/super.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index dbf16ad..8541c93 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -567,6 +567,7 @@ static void f2fs_put_super(struct super_block *sb)
>>
>>   	sb->s_fs_info = NULL;
>>   	brelse(sbi->raw_super_buf);
>> +	kfree(sbi->raw_super);
>>   	kfree(sbi);
>>   }
>>
>> @@ -1040,6 +1041,9 @@ static int read_raw_super_block(struct super_block *sb,
>>   	struct f2fs_super_block *super;
>>   	int err = 0;
>>
>> +	super = kzalloc(sizeof(struct f2fs_super_block), GFP_KERNEL);
>> +	if (!super)
>> +		return -ENOMEM;
>>   retry:
>>   	buffer = sb_bread(sb, block);
>>   	if (!buffer) {
>> @@ -1055,8 +1059,7 @@ retry:
>>   		}
>>   	}
>>
>> -	super = (struct f2fs_super_block *)
>> -		((char *)(buffer)->b_data + F2FS_SUPER_OFFSET);
>> +	memcpy(super, buffer->b_data + F2FS_SUPER_OFFSET, sizeof(*super));
>>
>>   	/* sanity checking of raw super */
>>   	if (sanity_check_raw_super(sb, super)) {
>> @@ -1090,14 +1093,17 @@ retry:
>>
>>   out:
>>   	/* No valid superblock */
>> -	if (!*raw_super)
>> +	if (!*raw_super) {
>> +		kfree(super);
>>   		return err;
>> +	}
>>
>>   	return 0;
>>   }
>>
>>   int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>   {
>> +	struct f2fs_super_block *super = F2FS_RAW_SUPER(sbi);
>>   	struct buffer_head *sbh = sbi->raw_super_buf;
>>   	struct buffer_head *bh;
>>   	int err;
>> @@ -1108,7 +1114,7 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>   		return -EIO;
>>
>>   	lock_buffer(bh);
>> -	memcpy(bh->b_data, sbh->b_data, sbh->b_size);
>> +	memcpy(bh->b_data + F2FS_SUPER_OFFSET, super, sizeof(*super));
>>   	WARN_ON(sbh->b_size != F2FS_BLKSIZE);
>>   	set_buffer_uptodate(bh);
>>   	set_buffer_dirty(bh);
>> @@ -1124,6 +1130,10 @@ int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover)
>>
>>   	/* write current valid superblock */
>>   	lock_buffer(sbh);
>> +	if (memcmp(sbh->b_data + F2FS_SUPER_OFFSET, super, sizeof(*super))) {
>> +		f2fs_msg(sbi->sb, KERN_INFO, "Write modified valid superblock");
>> +		memcpy(sbh->b_data + F2FS_SUPER_OFFSET, super, sizeof(*super));
>> +	}
>>   	set_buffer_dirty(sbh);
>>   	unlock_buffer(sbh);
>>
>> --
>> 2.4.9 (Apple Git-60)
>
>
>
> .
>


------------------------------------------------------------------------------