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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 11 Oct 2012 09:24:12 +1100
From:	NeilBrown <neilb@...e.de>
To:	김재극 <jaegeuk.kim@...sung.com>
Cc:	viro@...iv.linux.org.uk, 'Theodore Ts'o' <tytso@....edu>,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	chur.lee@...sung.com, cm224.lee@...sung.com,
	jooyoung.hwang@...sung.com
Subject: Re: [PATCH 05/16] f2fs: add checkpoint operations

On Fri, 05 Oct 2012 20:59:29 +0900 김재극 <jaegeuk.kim@...sung.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.

Regards,
NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)