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

[Jaegeuk Kim]

2012-10-11 (ë), 09:50 +1100, NeilBrown:
> On Fri, 05 Oct 2012 20:57:46 +0900 êìê <jaegeuk.kim@xxxxxxxxxxx> 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.
> 

Definitely it's right.
Thank you.

> 
> Regards,
> NeilBrown
> 
> 
[snip]

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