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

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] [day] [month] [year] [list]

Message-ID: <1350052261.2299.31.camel@kjgkr>
Date:	Fri, 12 Oct 2012 23:31:01 +0900
From:	Jaegeuk Kim <jaegeuk.kim@...il.com>
To:	NeilBrown <neilb@...e.de>
Cc:	±èÀç±Ø <jaegeuk.kim@...sung.com>,
	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 03/16] f2fs: add superblock and major in-memory
 structures

2012-10-11 (ëª©), 09:50 +1100, NeilBrown:
> On Fri, 05 Oct 2012 20:57:46 +0900 ê¹€ìž¬ê·¹ <jaegeuk.kim@...sung.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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/