linux-kernel - Re: [PATCH 08/11] VFS: add inode_dir

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: <173492340768.11072.6052736961769187676@noble.neil.brown.name>
Date: Mon, 23 Dec 2024 14:10:07 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Hillf Danton" <hdanton@...a.com>
Cc: "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Peter Zijlstra" <peterz@...radead.org>,
 "Linus Torvalds" <torvalds@...ux-foundation.org>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/11] VFS: add inode_dir_lock/unlock

On Sat, 21 Dec 2024, Hillf Danton wrote:
> On Fri, 20 Dec 2024 13:54:26 +1100 NeilBrown <neilb@...e.de>
> > During the transition from providing exclusive locking on the directory
> > for directory modifying operation to providing exclusive locking only on
> > the dentry with a shared lock on the directory - we need an alternate
> > way to provide exclusion on the directory for file systems which haven't
> > been converted.  This is provided by inode_dir_lock() and
> > inode_dir_inlock().
> > This uses a bit in i_state for locking, and wait_var_event_spinlock() for
> > waiting.
> > 
> Inventing anything like mutex sounds bad.

In general I would agree.  But when the cost of adding a mutex exceeds
the cost of using an alternate solution that only requires 2 bits, I
think the alternate solution is justified.

> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -722,6 +722,8 @@ struct inode {
> >  		void (*free_inode)(struct inode *);
> >  	};
> >  	struct file_lock_context	*i_flctx;
> > +
> > +	struct lockdep_map	i_dirlock_map;	/* For tracking I_DIR_LOCKED locks */
> 
> The cost of this map says no to any attempt inventing mutex in any form.
> 

"struct lockdep_map" is size-zero when lockdep is not enabled.  And when
it is enabled we accept the cost of larger structures to benefit from
deadlock detection.
So I don't think this is a sound argument.

Thanks for the review,
NeilBrown