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

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4909 bytes --]

On Sat, 13 Oct 2012 00:49:06 +0900 Jaegeuk Kim <jaegeuk.kim@gmail.com> wrote:

> 2012-10-11 (목), 09:24 +1100, NeilBrown:
> > On Fri, 05 Oct 2012 20:59:29 +0900 김재극 <jaegeuk.kim@samsung.com> 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,

I see.  I missed the fact that the current data summary blocks are always
written to the checkpoint area - I assumed they were being written back to
the SSA.

So it makes sense now and you are right - no change needed.

Thanks,
NeilBrown


> 
> > 
> > Regards,
> > NeilBrown
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]