Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
From: Ira Weiny <ira.weiny@intel.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-kernel@vger.kernel.org,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Dan Williams <dan.j.williams@intel.com>,
	Dave Chinner <david@fromorbit.com>,
	Christoph Hellwig <hch@lst.de>,
	"Theodore Y. Ts'o" <tytso@mit.edu>, Jan Kara <jack@suse.cz>,
	linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [RFC PATCH V2 07/12] fs: Add locking for a dynamic inode 'mode'
Date: Mon, 13 Jan 2020 16:20:05 -0800
Message-ID: <20200114002005.GA29860@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20200113221218.GM8247@magnolia>

On Mon, Jan 13, 2020 at 02:12:18PM -0800, Darrick J. Wong wrote:
> On Fri, Jan 10, 2020 at 11:29:37AM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>

[snip]

> >  
> >  The File Object
> >  ---------------
> > @@ -437,6 +459,8 @@ As of kernel 2.6.22, the following members are defined:
> >  		int (*atomic_open)(struct inode *, struct dentry *, struct file *,
> >  				   unsigned open_flag, umode_t create_mode);
> >  		int (*tmpfile) (struct inode *, struct dentry *, umode_t);
> > +		void (*lock_mode)(struct inode *);
> > +		void (*unlock_mode)(struct inode *);
> 
> Yikes.  "mode" has a specific meaning for inodes, and this lock isn't
> related to i_mode.  This lock protects aops from changing while an
> address space operation is in use.

Ah...  yea ok mode is a bad name.

> 
> >  	};
> >  
> >  Again, all methods are called without any locks being held, unless
> > @@ -584,6 +608,12 @@ otherwise noted.
> >  	atomically creating, opening and unlinking a file in given
> >  	directory.
> >  
> > +``lock_mode``
> > +	called to prevent operations which depend on the inode's mode from
> > +        proceeding should a mode change be in progress
> 
> "Inodes can't change mode, because files do not suddenly become
> directories". ;)

Yea sorry.

> 
> Oh, you meant "lock_XXXX is called to prevent a change in the pagecache
> mode from proceeding while there are address space operations in
> progress".  So these are really more aops get and put functions...

At first I actually did have aops get/put functions but this is really
protecting more than the aops vector because as Christoph said there are file
operations which need to be protected not just address space operations.

But I agree "mode" is a bad name...  Sorry...

> 
> > +``unlock_mode``
> > +	called when critical mode dependent operation is complete
> >  
> >  The Address Space Object
> >  ========================
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 7c9a5df5a597..ed6ab5303a24 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -55,18 +55,29 @@ EXPORT_SYMBOL(vfs_ioctl);
> >  static int ioctl_fibmap(struct file *filp, int __user *p)
> >  {
> >  	struct address_space *mapping = filp->f_mapping;
> > +	struct inode *inode = filp->f_inode;
> >  	int res, block;
> >  
> > +	lock_inode_mode(inode);
> > +
> >  	/* do we support this mess? */
> > -	if (!mapping->a_ops->bmap)
> > -		return -EINVAL;
> > -	if (!capable(CAP_SYS_RAWIO))
> > -		return -EPERM;
> > +	if (!mapping->a_ops->bmap) {
> > +		res = -EINVAL;
> > +		goto out;
> > +	}
> > +	if (!capable(CAP_SYS_RAWIO)) {
> > +		res = -EPERM;
> > +		goto out;
> 
> Why does the order of these checks change here?

I don't understand?  The order does not change we just can't return without
releasing the lock.  And to protect against bmap changing the lock needs to be
taken first.

[snip]

> >  
> > +static inline void lock_inode_mode(struct inode *inode)
> 
> inode_aops_get()?

Let me think on this.  This is not just getting a reference to the aops vector.
It is more than that...  and inode_get is not right either!  ;-P

> 
> > +{
> > +	WARN_ON_ONCE(inode->i_op->lock_mode &&
> > +		     !inode->i_op->unlock_mode);
> > +	if (inode->i_op->lock_mode)
> > +		inode->i_op->lock_mode(inode);
> > +}
> > +static inline void unlock_inode_mode(struct inode *inode)
> > +{
> > +	WARN_ON_ONCE(inode->i_op->unlock_mode &&
> > +		     !inode->i_op->lock_mode);
> > +	if (inode->i_op->unlock_mode)
> > +		inode->i_op->unlock_mode(inode);
> > +}
> > +
> >  static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
> >  				     struct iov_iter *iter)
> 
> inode_aops_put()?

...  something like that but not 'aops'...

Ira

> 
> --D
> 

  reply index

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 19:29 [RFC PATCH V2 00/12] Enable per-file/directory DAX operations V2 ira.weiny
2020-01-10 19:29 ` [RFC PATCH V2 01/12] fs/stat: Define DAX statx attribute ira.weiny
2020-01-15 11:37   ` Jan Kara
2020-01-15 17:38     ` Darrick J. Wong
2020-01-15 19:45       ` Ira Weiny
2020-01-15 20:10         ` Dan Williams
2020-01-15 22:38           ` Ira Weiny
2020-01-16  5:39             ` Darrick J. Wong
2020-01-16  6:05               ` Dan Williams
2020-01-16  6:18                 ` Darrick J. Wong
2020-01-16  6:25                   ` Dan Williams
2020-01-18  9:11                 ` Dave Chinner
2020-01-16 17:55               ` Ira Weiny
2020-01-16 18:04                 ` Darrick J. Wong
2020-01-16 18:52                   ` Ira Weiny
2020-01-16 22:19                     ` Darrick J. Wong
2020-01-17 11:58                     ` Jan Kara
2020-01-10 19:29 ` [RFC PATCH V2 02/12] fs/xfs: Isolate the physical DAX flag from effective ira.weiny
2020-01-10 19:29 ` [RFC PATCH V2 03/12] fs/xfs: Separate functionality of xfs_inode_supports_dax() ira.weiny
2020-01-10 19:29 ` [RFC PATCH V2 04/12] fs/xfs: Clean up DAX support check ira.weiny
2020-01-10 19:29 ` [RFC PATCH V2 05/12] fs: remove unneeded IS_DAX() check ira.weiny
2020-01-16  9:38   ` Jan Kara
2020-01-16 18:47     ` Ira Weiny
2020-01-10 19:29 ` [RFC PATCH V2 06/12] fs/xfs: Check if the inode supports DAX under lock ira.weiny
2020-01-10 19:29 ` [RFC PATCH V2 07/12] fs: Add locking for a dynamic inode 'mode' ira.weiny
2020-01-13 22:12   ` Darrick J. Wong
2020-01-14  0:20     ` Ira Weiny [this message]
2020-01-14  1:03       ` Darrick J. Wong
2020-01-15 19:08         ` Ira Weiny
2020-01-16  5:40           ` Darrick J. Wong
2020-01-16 18:54             ` Ira Weiny
2020-01-10 19:29 ` [RFC PATCH V2 08/12] fs/xfs: Add lock/unlock mode to xfs ira.weiny
2020-01-13 22:19   ` Darrick J. Wong
2020-01-14  0:35     ` Ira Weiny
2020-01-15  0:57       ` Ira Weiny
2020-01-15 23:52     ` Ira Weiny
2020-01-16  9:24   ` Jan Kara
2020-01-16 19:12     ` Ira Weiny
2020-01-10 19:29 ` [RFC PATCH V2 09/12] fs: Prevent mode change if file is mmap'ed ira.weiny
2020-01-13 22:22   ` Darrick J. Wong
2020-01-14  0:46     ` Ira Weiny
2020-01-14  1:30       ` Darrick J. Wong
2020-01-14 17:53         ` Ira Weiny
2020-01-15 11:34           ` Jan Kara
2020-01-15 18:24             ` Ira Weiny
2020-01-15 10:21   ` David Laight
2020-01-15 17:53     ` Ira Weiny
2020-01-10 19:29 ` [RFC PATCH V2 10/12] fs/xfs: Fix truncate up ira.weiny
2020-01-13 22:27   ` Darrick J. Wong
2020-01-14  0:40     ` Ira Weiny
2020-01-14  1:14       ` Darrick J. Wong
2020-01-14 19:00         ` Ira Weiny
2020-01-14 19:39           ` Ira Weiny
2020-01-10 19:29 ` [RFC PATCH V2 11/12] fs/xfs: Clean up locking in dax invalidate ira.weiny
2020-01-10 19:29 ` [RFC PATCH V2 12/12] fs/xfs: Allow toggle of effective DAX flag ira.weiny

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200114002005.GA29860@iweiny-DESK2.sc.intel.com \
    --to=ira.weiny@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=darrick.wong@oracle.com \
    --cc=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=jack@suse.cz \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git