On Fri, Feb 08, 2019 at 10:31:07PM +0200, Amir Goldstein wrote: > On Fri, Feb 8, 2019 at 10:17 PM J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > On Fri, Feb 08, 2019 at 10:02:43PM +0200, Amir Goldstein wrote: > > > On Fri, Feb 8, 2019 at 5:51 PM J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote: > > > > On Fri, Feb 08, 2019 at 04:45:46PM +0200, Amir Goldstein wrote: > > > > > - check_conflicting_open() is changed to use inode_is_open_for_read() > > > > > instead of checking d_count and i_count. > > > > > > > > Independently of the rest, I'd love to do away with those > > > > d_count/i_count checks. What's inode_is_open_for_read()? > > > > > > > > > > It would look maybe something like this: > > > > > > static inline bool file_is_open_for_read(const struct inode *file) > > > { > > > struct inode *inode = file_inode(file); > > > int countself = (file->f_mode & (FMODE_READ | FMODE_WRITE)) == > > > FMODE_READ) ? 1 : 0; > > > > > > return atomic_read(&inode->i_readcount) > countself; > > > } > > > > > > And it would allow for acquiring F_WRLCK lease if other > > > instances of inode are open O_PATH. > > > A slight change of semantics that seems harmless(?) > > > and will allow some flexibility. > > > > How did I not know about i_readcount? (Looking) I guess it would mean > > adding some dependence on CONFIG_IMA, hm. > > > > Yes, or we remove ifdef CONFIG_IMA from i_readcount. > I am not sure if the concern was size of struct inode > (shouldn't increase on 64bit arch) or the accounting on > open/close. The impact doesn't look significant (?).. Looks like the original patch was d984ea604943bb "fs: move i_readcount". I did some googling around and looked at the discussion summarized by https://lwn.net/Articles/410895/ but can't find useful discussion of i_readcount impact. Looks like CONFIG_IMA is on in Fedora and RHEL, for what it's worth. Maybe something like this? --b. commit 02cfda99ed8c Author: J. Bruce Fields <bfields@xxxxxxxxxx> Date: Thu Feb 14 15:02:02 2019 -0500 locks: use i_readcount to detect lease conflicts The lease code currently uses the inode and dentry refcounts to detect whether someone has a file open for read. This seems fragile. Use i_readcount instead. Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx> diff --git a/fs/locks.c b/fs/locks.c index ff6af2c32601..299abad65545 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1769,8 +1769,7 @@ check_conflicting_open(const struct dentry *dentry, const long arg, int flags) if ((arg == F_RDLCK) && inode_is_open_for_write(inode)) return -EAGAIN; - if ((arg == F_WRLCK) && ((d_count(dentry) > 1) || - (atomic_read(&inode->i_count) > 1))) + if ((arg == F_WRLCK) && (atomic_read(&inode->i_readcount) > 1)) ret = -EAGAIN; return ret; diff --git a/include/linux/fs.h b/include/linux/fs.h index 29d8e2cfed0e..e862de682da9 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -676,7 +676,7 @@ struct inode { atomic_t i_count; atomic_t i_dio_count; atomic_t i_writecount; -#ifdef CONFIG_IMA +#if defined(CONFIG_IMA) || (defined_CONFIG_FILE_LOCKING) atomic_t i_readcount; /* struct files open RO */ #endif const struct file_operations *i_fop; /* former ->i_op->default_file_ops */ @@ -2869,7 +2869,7 @@ static inline bool inode_is_open_for_write(const struct inode *inode) return atomic_read(&inode->i_writecount) > 0; } -#ifdef CONFIG_IMA +#if defined(CONFIG_IMA) || defined(CONFIG_FILE_LOCKING) static inline void i_readcount_dec(struct inode *inode) { BUG_ON(!atomic_read(&inode->i_readcount));