2012-10-11 (목), 09:50 +1100, NeilBrown:
> On Fri, 05 Oct 2012 20:57:46 +0900 김재극 <jaegeuk....@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.
>
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
