linux-ext4 - Re: [RFC PATCH V2 08/12] fs/xfs: Add lock/unlock mode to xfs

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: <20200114003521.GB29860@iweiny-DESK2.sc.intel.com>
Date:   Mon, 13 Jan 2020 16:35:21 -0800
From:   Ira Weiny <ira.weiny@...el.com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     linux-kernel@...r.kernel.org,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Dan Williams <dan.j.williams@...el.com>,
        Dave Chinner <david@...morbit.com>,
        Christoph Hellwig <hch@....de>,
        "Theodore Y. Ts'o" <tytso@....edu>, Jan Kara <jack@...e.cz>,
        linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: [RFC PATCH V2 08/12] fs/xfs: Add lock/unlock mode to xfs

On Mon, Jan 13, 2020 at 02:19:57PM -0800, Darrick J. Wong wrote:
> On Fri, Jan 10, 2020 at 11:29:38AM -0800, ira.weiny@...el.com wrote:
> > From: Ira Weiny <ira.weiny@...el.com>

[snip]

> >  
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 401da197f012..e8fd95b75e5b 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
> >   *
> >   * Basic locking order:
> >   *
> > - * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
> > + * i_rwsem -> i_dax_sem -> i_mmap_lock -> page_lock -> i_ilock
> 
> Mmmmmm, more locks.  Can we skip the extra lock if CONFIG_FSDAX=n or if
> the filesystem devices don't support DAX at all?

I'll look into it.

> 
> Also, I don't think we're actually following the i_rwsem -> i_daxsem
> order in fallocate, and possibly elsewhere too?

I'll have to verify.  It took a lot of iterations to get the order working so
I'm not going to claim perfection.

> 
> Does the vfs have to take the i_dax_sem to do remapping things like
> reflink?  (Pretend that reflink and dax are compatible for the moment)

Honestly I can't say for sure.  For this series I was careful to exclude
reflink from the locking requirement.

[snip]

> >  
> >  #define XFS_LOCK_FLAGS \
> >  	{ XFS_IOLOCK_EXCL,	"IOLOCK_EXCL" }, \
> > @@ -289,7 +295,9 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
> >  	{ XFS_ILOCK_EXCL,	"ILOCK_EXCL" }, \
> >  	{ XFS_ILOCK_SHARED,	"ILOCK_SHARED" }, \
> >  	{ XFS_MMAPLOCK_EXCL,	"MMAPLOCK_EXCL" }, \
> > -	{ XFS_MMAPLOCK_SHARED,	"MMAPLOCK_SHARED" }
> > +	{ XFS_MMAPLOCK_SHARED,	"MMAPLOCK_SHARED" }, \
> > +	{ XFS_DAX_EXCL,   	"DAX_EXCL" }, \
> 
> Whitespace between the comma & string.

Fixed.

[snip]

> > +
> >  static const struct inode_operations xfs_dir_inode_operations = {
> >  	.create			= xfs_vn_create,
> >  	.lookup			= xfs_vn_lookup,
> > @@ -1372,7 +1394,7 @@ xfs_setup_iops(
> >  
> >  	switch (inode->i_mode & S_IFMT) {
> >  	case S_IFREG:
> > -		inode->i_op = &xfs_inode_operations;
> > +		inode->i_op = &xfs_reg_inode_operations;
> 
> xfs_file_inode_operations?

Sounds better.  Fixed.

Ira