Re: [PATCH 03/16] f2fs: add superblock and major in-memory structures

[NeilBrown <neilb@suse.de>]

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

On Fri, 05 Oct 2012 20:57:46 +0900 김재극 <jaegeuk.kim@samsung.com> wrote:


> +static inline unsigned int curseg_segno(struct f2fs_sb_info *sbi,
> +		int type)
> +{
> +	struct curseg_info *curseg = CURSEG_I(sbi, type);
> +	unsigned int segno;
> +	mutex_lock(&curseg->curseg_mutex);
> +	segno = curseg->segno;
> +	mutex_unlock(&curseg->curseg_mutex);
> +	return segno;
> +}
> +
> +static inline unsigned char curseg_alloc_type(struct f2fs_sb_info *sbi,
> +		int type)
> +{
> +	struct curseg_info *curseg = CURSEG_I(sbi, type);
> +	unsigned char a_type;
> +	mutex_lock(&curseg->curseg_mutex);
> +	a_type = curseg->alloc_type;
> +	mutex_unlock(&curseg->curseg_mutex);
> +	return a_type;
> +}
> +
> +static inline unsigned short curseg_blkoff(struct f2fs_sb_info *sbi, int type)
> +{
> +	struct curseg_info *curseg = CURSEG_I(sbi, type);
> +	unsigned short blkoff;
> +	mutex_lock(&curseg->curseg_mutex);
> +	blkoff = curseg->next_blkoff;
> +	mutex_unlock(&curseg->curseg_mutex);
> +	return blkoff;
> +}

Taking a mutex just to extract a small number from a structure is pointless.
alloc_type, next_blkoff and segno are char, short, and int.  All of these can
be read atomically, so a lock gains you nothing.

In checkpoint.c we have
	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);
	}

which will take and drop that same lock 3 times in quick succession, and then
do it again for 3 other locks (And there is another loop which does it for
the other 3 cursegs).

If you do need some locking here, I think you need to take the lock once per
loop iteration so the 3 values are consistent, not once for each value.


Regards,
NeilBrown


> +
> +static inline void check_seg_range(struct f2fs_sb_info *sbi, unsigned int segno)
> +{
> +	unsigned int end_segno = SM_I(sbi)->segment_count - 1;
> +	BUG_ON(segno > end_segno);
> +}
> +
> +/*
> + * This function is used for only debugging.
> + * NOTE: In future, we have to remove this function.
> + */
> +static inline void verify_block_addr(struct f2fs_sb_info *sbi, block_t blk_addr)
> +{
> +	struct f2fs_sm_info *sm_info = SM_I(sbi);
> +	block_t total_blks = sm_info->segment_count << sbi->log_blocks_per_seg;
> +	block_t start_addr = sm_info->seg0_blkaddr;
> +	block_t end_addr = start_addr + total_blks - 1;
> +	BUG_ON(blk_addr < start_addr);
> +	BUG_ON(blk_addr > end_addr);
> +}
> +
> +/**
> + * Summary block is always treated as invalid block
> + */
> +static inline void check_block_count(struct f2fs_sb_info *sbi,
> +		int segno, struct f2fs_sit_entry *raw_sit)
> +{
> +	struct f2fs_sm_info *sm_info = SM_I(sbi);
> +	unsigned int end_segno = sm_info->segment_count - 1;
> +	int valid_blocks = 0;
> +	int i;
> +
> +	/* check segment usage */
> +	BUG_ON(GET_SIT_VBLOCKS(raw_sit) > sbi->blocks_per_seg);
> +
> +	/* check boundary of a given segment number */
> +	BUG_ON(segno > end_segno);
> +
> +	/* check bitmap with valid block count */
> +	for (i = 0; i < sbi->blocks_per_seg; i++)
> +		if (f2fs_test_bit(i, raw_sit->valid_map))
> +			valid_blocks++;
> +	BUG_ON(GET_SIT_VBLOCKS(raw_sit) != valid_blocks);
> +}
> +
> +static inline pgoff_t current_sit_addr(struct f2fs_sb_info *sbi,
> +						unsigned int start)
> +{
> +	struct sit_info *sit_i = SIT_I(sbi);
> +	unsigned int offset = SIT_BLOCK_OFFSET(sit_i, start);
> +	block_t blk_addr = sit_i->sit_base_addr + offset;
> +
> +	check_seg_range(sbi, start);
> +
> +	/* calculate sit block address */
> +	if (f2fs_test_bit(offset, sit_i->sit_bitmap))
> +		blk_addr += sit_i->sit_blocks;
> +
> +	return blk_addr;
> +}
> +
> +static inline pgoff_t next_sit_addr(struct f2fs_sb_info *sbi,
> +						pgoff_t block_addr)
> +{
> +	struct sit_info *sit_i = SIT_I(sbi);
> +	block_addr -= sit_i->sit_base_addr;
> +	if (block_addr < sit_i->sit_blocks)
> +		block_addr += sit_i->sit_blocks;
> +	else
> +		block_addr -= sit_i->sit_blocks;
> +
> +	return block_addr + sit_i->sit_base_addr;
> +}
> +
> +static inline void set_to_next_sit(struct sit_info *sit_i, unsigned int start)
> +{
> +	unsigned int block_off = SIT_BLOCK_OFFSET(sit_i, start);
> +
> +	if (f2fs_test_bit(block_off, sit_i->sit_bitmap))
> +		f2fs_clear_bit(block_off, sit_i->sit_bitmap);
> +	else
> +		f2fs_set_bit(block_off, sit_i->sit_bitmap);
> +}
> +
> +static inline uint64_t div64_64(uint64_t dividend, uint64_t divisor)
> +{
> +	uint32_t d = divisor;
> +
> +	if (divisor > 0xffffffffUll) {
> +		unsigned int shift = fls(divisor >> 32);
> +		d = divisor >> shift;
> +		dividend >>= shift;
> +	}
> +
> +	if (dividend >> 32)
> +		do_div(dividend, d);
> +	else
> +		dividend = (uint32_t) dividend / d;
> +
> +	return dividend;
> +}
> +
> +static inline unsigned long long get_mtime(struct f2fs_sb_info *sbi)
> +{
> +	struct sit_info *sit_i = SIT_I(sbi);
> +	return sit_i->elapsed_time + CURRENT_TIME_SEC.tv_sec -
> +						sit_i->mounted_time;
> +}
> +
> +static inline void set_summary(struct f2fs_summary *sum, nid_t nid,
> +			unsigned int ofs_in_node, unsigned char version)
> +{
> +	sum->nid = cpu_to_le32(nid);
> +	sum->ofs_in_node = cpu_to_le16(ofs_in_node);
> +	sum->version = version;
> +}
> +
> +static inline block_t start_sum_block(struct f2fs_sb_info *sbi)
> +{
> +	return __start_cp_addr(sbi) +
> +		le32_to_cpu(F2FS_CKPT(sbi)->cp_pack_start_sum);
> +}
> +
> +static inline block_t sum_blk_addr(struct f2fs_sb_info *sbi, int base, int type)
> +{
> +	return __start_cp_addr(sbi) +
> +		le32_to_cpu(F2FS_CKPT(sbi)->cp_pack_total_block_count)
> +				- (base + 1) + type;
> +}


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