Re: [PATCH 05/16] f2fs: add checkpoint operations

[Jaegeuk Kim]

2012-10-11 (ë), 09:24 +1100, NeilBrown:
> On Fri, 05 Oct 2012 20:59:29 +0900 êìê <jaegeuk.kim@xxxxxxxxxxx> wrote:
> 
> > +static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
> > +{
> > +	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
> > +	nid_t last_nid = 0;
> > +	int nat_upd_blkoff[3];
> > +	block_t start_blk;
> > +	struct page *cp_page;
> > +	unsigned int data_sum_blocks, orphan_blocks;
> > +	void *kaddr;
> > +	__u32 crc32 = 0;
> > +	int i;
> > +
> > +	/* Flush all the NAT/SIT pages */
> > +	while (get_pages(sbi, F2FS_DIRTY_META))
> > +		sync_meta_pages(sbi, META, LONG_MAX);
> > +
> > +	next_free_nid(sbi, &last_nid);
> > +
> > +	/*
> > +	 * modify checkpoint
> > +	 * version number is already updated
> > +	 */
> > +	ckpt->elapsed_time = cpu_to_le64(get_mtime(sbi));
> > +	ckpt->valid_block_count = cpu_to_le64(valid_user_blocks(sbi));
> > +	ckpt->free_segment_count = cpu_to_le32(free_segments(sbi));
> > +	for (i = 0; i < 3; i++) {
> > +		ckpt->cur_node_segno[i] =
> > +			cpu_to_le32(curseg_segno(sbi, i + CURSEG_HOT_NODE));
> > +		ckpt->cur_node_blkoff[i] =
> > +			cpu_to_le16(curseg_blkoff(sbi, i + CURSEG_HOT_NODE));
> > +		nat_upd_blkoff[i] = NM_I(sbi)->nat_upd_blkoff[i];
> > +		ckpt->nat_upd_blkoff[i] = cpu_to_le16(nat_upd_blkoff[i]);
> > +		ckpt->alloc_type[i + CURSEG_HOT_NODE] =
> > +				curseg_alloc_type(sbi, i + CURSEG_HOT_NODE);
> > +	}
> > +	for (i = 0; i < 3; i++) {
> > +		ckpt->cur_data_segno[i] =
> > +			cpu_to_le32(curseg_segno(sbi, i + CURSEG_HOT_DATA));
> > +		ckpt->cur_data_blkoff[i] =
> > +			cpu_to_le16(curseg_blkoff(sbi, i + CURSEG_HOT_DATA));
> > +		ckpt->alloc_type[i + CURSEG_HOT_DATA] =
> > +				curseg_alloc_type(sbi, i + CURSEG_HOT_DATA);
> > +	}
> > +
> > +	ckpt->valid_node_count = cpu_to_le32(valid_node_count(sbi));
> > +	ckpt->valid_inode_count = cpu_to_le32(valid_inode_count(sbi));
> > +	ckpt->next_free_nid = cpu_to_le32(last_nid);
> > +
> > +	/* 2 cp  + n data seg summary + orphan inode blocks */
> > +	data_sum_blocks = npages_for_summary_flush(sbi);
> > +	if (data_sum_blocks < 3)
> > +		ckpt->ckpt_flags |= CP_COMPACT_SUM_FLAG;
> > +	else
> > +		ckpt->ckpt_flags &= (~CP_COMPACT_SUM_FLAG);
> > +
> > +	orphan_blocks = (sbi->n_orphans + F2FS_ORPHANS_PER_BLOCK - 1)
> > +					/ F2FS_ORPHANS_PER_BLOCK;
> > +	ckpt->cp_pack_start_sum = 1 + orphan_blocks;
> > +	ckpt->cp_pack_total_block_count = 2 + data_sum_blocks + orphan_blocks;
> 
> This looks a bit weird to me, though I might be misunderstanding something.
> 
> data_sum_blocks is either 1, 2, or 3.
> "3" actually means "at least 3".
> 
> If it is 3, you choose not to set CP_COMPACT_SUM_FLAG.  In that case the NAT
> and SIT journal entries go into SSA blocks, not into the checkpoint at all.
> So in that case, zero blocks of the checkpoint are used for journalling.  Yet
> you still add data_sum_blocks (==3) to the cp_pack_total_block_count (and
> later to the start block).
> Is that really what you want to do?  Leave 3 empty blocks?
> 
> I would suggest changing npages_for_summary_flush to return 0 if the number
> of blocks needed would be more than three, and set CP_COMPACT_SUM_FLAG only
> when data_sum_blocks > 0.
> 
> I don't know if you would need to make a corresponding change to the recovery
> code, I haven't fully examined that yet.

Ok, let me explain about CP_COMPACT_SUM_FLAG.
Let's assume that there are some journal entries and data summaries.
Note that this scenario is not from the umount procedure.

Basically f2fs writes three data summary blocks for current active logs
inside the checkpoint pack.
And NAT and SIT journal entries are stored in hot and cold data summary
blocks.
So, if the CP_COMPACT_SUM_FLAG is not set, f2fs writes the checkpoint
pack like this.

[CP 0]
[Orphan blocks]
[Hot sum block w/ NAT journal]
[Warm sum block]
[Cold sum block w/ SIT journal]
[CP 0']

But, if the CP_COMPACT_SUM_FLAG is set, the checkpoint pack consists of
1 or 2 summary blocks as follows.

[CP 0]
[Orphan blocks]
[summary entries w/ NAT and SIT journal]
[CP 0']

or,

[CP 0]
[Orphan blocks]
[summary entries]
[summary entries w/ NAT and SIT journal]
[CP 0']

So, I think it needs no change.
Any idea?
Thanks,

> 
> Regards,
> NeilBrown

-- 
Jaegeuk Kim
Samsung

--
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/