linux-kernel - Re: [PATCH 04/27] ext2: Add ext2_sb

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]

Message-ID: <20091105135521.GD12770@duck.suse.cz>
Date:	Thu, 5 Nov 2009 14:55:21 +0100
From:	Jan Kara <jack@...e.cz>
To:	Jan Blunck <jblunck@...e.de>
Cc:	Andi Kleen <andi@...stfloor.org>, linux-fsdevel@...r.kernel.org,
	Matthew Wilcox <matthew@....cx>, linux-kernel@...r.kernel.org,
	Jan Kara <jack@...e.cz>, Al Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <ak@...ux.intel.com>,
	Christoph Hellwig <hch@....de>,
	Pekka Enberg <penberg@...helsinki.fi>,
	Andreas Dilger <adilger@....com>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 04/27] ext2: Add ext2_sb_info mutex

On Mon 02-11-09 17:57:52, Jan Blunck wrote:
> On Mon, Nov 02, Andi Kleen wrote:
> 
> > > @@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> > >  	sbi->s_sb_block = sb_block;
> > >  
> > >  	/*
> > > +	 * mutex for protection of modifications of the superblock while being
> > > +	 * write out by ext2_write_super() or ext2_sync_fs().
> > > +	 */
> > > +	mutex_init(&sbi->s_mutex);
> > 
> > I didn't go over all the code paths in detail, but if you replace
> > the BKL with a mutex that is hold over a longer write-out sleep
> > period you potentially limit IO parallelism a lot.
> 
> Right. I converted it to be a spinlock and unlock before calling
> ext2_sync_super().
> 
> What do you think?
  The patch is generally fine. I have just a few minor comments below:

> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index 5af1775..70c326c 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -52,8 +52,10 @@ void ext2_error (struct super_block * sb, const char * function,
>  	struct ext2_super_block *es = sbi->s_es;
>  
>  	if (!(sb->s_flags & MS_RDONLY)) {
> +		spin_lock(&sbi->s_lock);
>  		sbi->s_mount_state |= EXT2_ERROR_FS;
>  		es->s_state |= cpu_to_le16(EXT2_ERROR_FS);
> +		/* drops sbi->s_lock */
>  		ext2_sync_super(sb, es);
   I don't like this dropping of spinlock inside ext2_sync_super. Can we
just drop it here and retake it in ext2_sync_super? It's by far not a
performance critical path so it should not really matter.

> diff --git a/include/linux/ext2_fs_sb.h b/include/linux/ext2_fs_sb.h
> index 1cdb663..0d20278 100644
> --- a/include/linux/ext2_fs_sb.h
> +++ b/include/linux/ext2_fs_sb.h
> @@ -106,6 +106,8 @@ struct ext2_sb_info {
>  	spinlock_t s_rsv_window_lock;
>  	struct rb_root s_rsv_window_root;
>  	struct ext2_reserve_window_node s_rsv_window_head;
> +	/* protect against concurrent modifications of this structure */
> +	spinlock_t s_lock;
>  };
  As I'm reading the code s_lock protects some of the fieds but definitely
not all. I'd say it protects s_mount_state, s_blocks_last, s_overhead_last,
and a content of superblock's buffer pointed to by sbi->s_es. The rest just
either does not change during lifetime of the filesystem or has different
locks (either s_umount semaphore or other spinlocks).

								Honza

-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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/