diff mbox series

[4/8] btrfs: Introduce btrfs_inode_lock()/unlock()

Message ID 20200622162017.21773-5-rgoldwyn@suse.de (mailing list archive)
State New, archived
Headers show
Series Rearrange inode locking | expand

Commit Message

Goldwyn Rodrigues June 22, 2020, 4:20 p.m. UTC 
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

btrfs_inode_lock/unlock() acquires the inode->i_rwsem depending on the
flags passed. ilock_flags determines the type of lock to be taken:

BTRFS_ILOCK_SHARED - for shared locks, for possible parallel DIO
BTRFS_ILOCK_TRY - for the RWF_NOWAIT sequence

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ctree.h |  8 ++++++++
 fs/btrfs/file.c  | 51 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 50 insertions(+), 9 deletions(-)

Comments

David Sterba June 24, 2020, 4:19 p.m. UTC | #1 
On Mon, Jun 22, 2020 at 11:20:13AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> btrfs_inode_lock/unlock() acquires the inode->i_rwsem depending on the
> flags passed. ilock_flags determines the type of lock to be taken:
> 
> BTRFS_ILOCK_SHARED - for shared locks, for possible parallel DIO
> BTRFS_ILOCK_TRY - for the RWF_NOWAIT sequence
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ctree.h |  8 ++++++++
>  fs/btrfs/file.c  | 51 +++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 161533040978..346fea668ca0 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2953,6 +2953,14 @@ void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
>  			       struct btrfs_ioctl_balance_args *bargs);
>  
>  /* file.c */
> +
> +/* ilock flags definition */
> +#define BTRFS_ILOCK_SHARED	0x1
> +#define BTRFS_ILOCK_TRY 	0x2

Please use enums and add them to a new file inode.h.

> +int btrfs_inode_lock(struct inode *inode, int ilock_flags);
> +void btrfs_inode_unlock(struct inode *inode, int ilock_flags);
> +
>  int __init btrfs_auto_defrag_init(void);
>  void __cold btrfs_auto_defrag_exit(void);
>  int btrfs_add_inode_defrag(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index ba7c3b2cf1c5..1a9a0a9e4b3d 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -70,6 +70,37 @@ static int __compare_inode_defrag(struct inode_defrag *defrag1,
>  		return 0;
>  }
>  
> +int btrfs_inode_lock(struct inode *inode, int ilock_flags)
> +void btrfs_inode_unlock(struct inode *inode, int ilock_flags)

And the helpers should be in inode.c, not file.c
Goldwyn Rodrigues June 25, 2020, 5:34 p.m. UTC | #2 
On 18:19 24/06, David Sterba wrote:
> On Mon, Jun 22, 2020 at 11:20:13AM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > btrfs_inode_lock/unlock() acquires the inode->i_rwsem depending on the
> > flags passed. ilock_flags determines the type of lock to be taken:
> > 
> > BTRFS_ILOCK_SHARED - for shared locks, for possible parallel DIO
> > BTRFS_ILOCK_TRY - for the RWF_NOWAIT sequence
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >  fs/btrfs/ctree.h |  8 ++++++++
> >  fs/btrfs/file.c  | 51 +++++++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 50 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 161533040978..346fea668ca0 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -2953,6 +2953,14 @@ void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
> >  			       struct btrfs_ioctl_balance_args *bargs);
> >  
> >  /* file.c */
> > +
> > +/* ilock flags definition */
> > +#define BTRFS_ILOCK_SHARED	0x1
> > +#define BTRFS_ILOCK_TRY 	0x2
> 
> Please use enums and add them to a new file inode.h.

These are bitwise flags. Wouldn't it be ugly if you have to set the flag
with:

flags |= (1 << BTRFS_ILOCK_TRY);

rather than

flags |= BTRFS_ILOCK_TRY;

> 
> > +int btrfs_inode_lock(struct inode *inode, int ilock_flags);
> > +void btrfs_inode_unlock(struct inode *inode, int ilock_flags);
> > +
> >  int __init btrfs_auto_defrag_init(void);
> >  void __cold btrfs_auto_defrag_exit(void);
> >  int btrfs_add_inode_defrag(struct btrfs_trans_handle *trans,
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index ba7c3b2cf1c5..1a9a0a9e4b3d 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -70,6 +70,37 @@ static int __compare_inode_defrag(struct inode_defrag *defrag1,
> >  		return 0;
> >  }
> >  
> > +int btrfs_inode_lock(struct inode *inode, int ilock_flags)
> > +void btrfs_inode_unlock(struct inode *inode, int ilock_flags)
> 
> And the helpers should be in inode.c, not file.c

Yes, inode.c is more appropriate. Perhaps I should change other calls to
inode_lock(inode) to btrfs_inode_lock(inode, 0) for uniformity.
Hans van Kranenburg June 26, 2020, 11:34 a.m. UTC | #3 
On 6/25/20 7:34 PM, Goldwyn Rodrigues wrote:
> On 18:19 24/06, David Sterba wrote:
>> On Mon, Jun 22, 2020 at 11:20:13AM -0500, Goldwyn Rodrigues wrote:
>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>
>>> btrfs_inode_lock/unlock() acquires the inode->i_rwsem depending on the
>>> flags passed. ilock_flags determines the type of lock to be taken:
>>>
>>> BTRFS_ILOCK_SHARED - for shared locks, for possible parallel DIO
>>> BTRFS_ILOCK_TRY - for the RWF_NOWAIT sequence
>>>
>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>> ---
>>>  fs/btrfs/ctree.h |  8 ++++++++
>>>  fs/btrfs/file.c  | 51 +++++++++++++++++++++++++++++++++++++++---------
>>>  2 files changed, 50 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index 161533040978..346fea668ca0 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -2953,6 +2953,14 @@ void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
>>>  			       struct btrfs_ioctl_balance_args *bargs);
>>>  
>>>  /* file.c */
>>> +
>>> +/* ilock flags definition */
>>> +#define BTRFS_ILOCK_SHARED	0x1
>>> +#define BTRFS_ILOCK_TRY 	0x2
>>
>> Please use enums and add them to a new file inode.h.
> 
> These are bitwise flags.

I suspect that in that case it's more common to do:

#define BTRFS_ILOCK_SHARED	(1 << 0)
#define BTRFS_ILOCK_TRY 	(1 << 1)

> Wouldn't it be ugly if you have to set the flag
> with:
> 
> flags |= (1 << BTRFS_ILOCK_TRY);
> 
> rather than
> 
> flags |= BTRFS_ILOCK_TRY;
> 
>>
>>> +int btrfs_inode_lock(struct inode *inode, int ilock_flags);
>>> +void btrfs_inode_unlock(struct inode *inode, int ilock_flags);
>>> +
>>>  int __init btrfs_auto_defrag_init(void);
>>>  void __cold btrfs_auto_defrag_exit(void);
>>>  int btrfs_add_inode_defrag(struct btrfs_trans_handle *trans,
>>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>>> index ba7c3b2cf1c5..1a9a0a9e4b3d 100644
>>> --- a/fs/btrfs/file.c
>>> +++ b/fs/btrfs/file.c
>>> @@ -70,6 +70,37 @@ static int __compare_inode_defrag(struct inode_defrag *defrag1,
>>>  		return 0;
>>>  }
>>>  
>>> +int btrfs_inode_lock(struct inode *inode, int ilock_flags)
>>> +void btrfs_inode_unlock(struct inode *inode, int ilock_flags)
>>
>> And the helpers should be in inode.c, not file.c
> 
> Yes, inode.c is more appropriate. Perhaps I should change other calls to
> inode_lock(inode) to btrfs_inode_lock(inode, 0) for uniformity.
> 

Hans
David Sterba June 26, 2020, 11:59 a.m. UTC | #4 
On Fri, Jun 26, 2020 at 01:34:05PM +0200, Hans van Kranenburg wrote:
> On 6/25/20 7:34 PM, Goldwyn Rodrigues wrote:
> > On 18:19 24/06, David Sterba wrote:
> >> On Mon, Jun 22, 2020 at 11:20:13AM -0500, Goldwyn Rodrigues wrote:
> >>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >>>
> >>> btrfs_inode_lock/unlock() acquires the inode->i_rwsem depending on the
> >>> flags passed. ilock_flags determines the type of lock to be taken:
> >>>
> >>> BTRFS_ILOCK_SHARED - for shared locks, for possible parallel DIO
> >>> BTRFS_ILOCK_TRY - for the RWF_NOWAIT sequence
> >>>
> >>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >>> ---
> >>>  fs/btrfs/ctree.h |  8 ++++++++
> >>>  fs/btrfs/file.c  | 51 +++++++++++++++++++++++++++++++++++++++---------
> >>>  2 files changed, 50 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> >>> index 161533040978..346fea668ca0 100644
> >>> --- a/fs/btrfs/ctree.h
> >>> +++ b/fs/btrfs/ctree.h
> >>> @@ -2953,6 +2953,14 @@ void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
> >>>  			       struct btrfs_ioctl_balance_args *bargs);
> >>>  
> >>>  /* file.c */
> >>> +
> >>> +/* ilock flags definition */
> >>> +#define BTRFS_ILOCK_SHARED	0x1
> >>> +#define BTRFS_ILOCK_TRY 	0x2
> >>
> >> Please use enums and add them to a new file inode.h.
> > 
> > These are bitwise flags.
> 
> I suspect that in that case it's more common to do:
> 
> #define BTRFS_ILOCK_SHARED	(1 << 0)
> #define BTRFS_ILOCK_TRY 	(1 << 1)

Yeah, that looks more clear that it's meant for bitwise operations.

We don't have any other enum for bitwise flags so define is ok for now.
David Sterba June 30, 2020, 8:34 a.m. UTC | #5 
On Mon, Jun 22, 2020 at 11:20:13AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> btrfs_inode_lock/unlock() acquires the inode->i_rwsem depending on the
> flags passed. ilock_flags determines the type of lock to be taken:
> 
> BTRFS_ILOCK_SHARED - for shared locks, for possible parallel DIO
> BTRFS_ILOCK_TRY - for the RWF_NOWAIT sequence
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ctree.h |  8 ++++++++
>  fs/btrfs/file.c  | 51 +++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 161533040978..346fea668ca0 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2953,6 +2953,14 @@ void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
>  			       struct btrfs_ioctl_balance_args *bargs);
>  
>  /* file.c */
> +
> +/* ilock flags definition */
> +#define BTRFS_ILOCK_SHARED	0x1
> +#define BTRFS_ILOCK_TRY 	0x2
> +
> +int btrfs_inode_lock(struct inode *inode, int ilock_flags);
> +void btrfs_inode_unlock(struct inode *inode, int ilock_flags);

There's another lock in btrfs_inode, and the locking functions added
here have the 'btrfs_' prefix, yet take the VFS inode structure. This is
confusing.

The flags are not btrfs-specific so it could be even a generic VFS
helper but for now I think it can be in fs/btrfs until it's finalized.
diff mbox series

Patch

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 161533040978..346fea668ca0 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2953,6 +2953,14 @@  void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
 			       struct btrfs_ioctl_balance_args *bargs);
 
 /* file.c */
+
+/* ilock flags definition */
+#define BTRFS_ILOCK_SHARED	0x1
+#define BTRFS_ILOCK_TRY 	0x2
+
+int btrfs_inode_lock(struct inode *inode, int ilock_flags);
+void btrfs_inode_unlock(struct inode *inode, int ilock_flags);
+
 int __init btrfs_auto_defrag_init(void);
 void __cold btrfs_auto_defrag_exit(void);
 int btrfs_add_inode_defrag(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index ba7c3b2cf1c5..1a9a0a9e4b3d 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -70,6 +70,37 @@  static int __compare_inode_defrag(struct inode_defrag *defrag1,
 		return 0;
 }
 
+int btrfs_inode_lock(struct inode *inode, int ilock_flags)
+{
+	if (ilock_flags & BTRFS_ILOCK_SHARED) {
+		if (ilock_flags & BTRFS_ILOCK_TRY) {
+			if (!inode_trylock_shared(inode))
+				return -EAGAIN;
+			else
+				return 0;
+		}
+		inode_lock_shared(inode);
+	} else {
+		if (ilock_flags & BTRFS_ILOCK_TRY) {
+			if (!inode_trylock(inode))
+				return -EAGAIN;
+			else
+				return 0;
+		}
+		inode_lock(inode);
+	}
+	return 0;
+}
+
+void btrfs_inode_unlock(struct inode *inode, int ilock_flags)
+{
+	if (ilock_flags & BTRFS_ILOCK_SHARED)
+		inode_unlock_shared(inode);
+	else
+		inode_unlock(inode);
+
+}
+
 /* pop a record for an inode into the defrag tree.  The lock
  * must be held already
  *
@@ -1978,6 +2009,7 @@  static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	ssize_t num_written = 0;
 	const bool sync = iocb->ki_flags & IOCB_DSYNC;
 	ssize_t err;
+	int ilock_flags = 0;
 
 	/*
 	 * If BTRFS flips readonly due to some impossible error
@@ -1992,12 +2024,12 @@  static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	    (iocb->ki_flags & IOCB_NOWAIT))
 		return -EOPNOTSUPP;
 
-	if (iocb->ki_flags & IOCB_NOWAIT) {
-		if (!inode_trylock(inode))
-			return -EAGAIN;
-	} else {
-		inode_lock(inode);
-	}
+	if (iocb->ki_flags & IOCB_NOWAIT)
+		ilock_flags |= BTRFS_ILOCK_TRY;
+
+	err = btrfs_inode_lock(inode, ilock_flags);
+	if (err < 0)
+		goto out;
 
 	err = btrfs_write_check(iocb, from);
 	if (err <= 0) {
@@ -2014,7 +2046,7 @@  static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 		num_written = btrfs_buffered_write(iocb, from);
 	}
 
-	inode_unlock(inode);
+	btrfs_inode_unlock(inode, ilock_flags);
 
 	/*
 	 * We also have to set last_sub_trans to the current log transid,
@@ -3547,9 +3579,10 @@  static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to)
 	if (is_sync_kiocb(iocb))
 		flags |= IOMAP_DIOF_WAIT_FOR_COMPLETION;
 
-	inode_lock_shared(inode);
+	btrfs_inode_lock(inode, BTRFS_ILOCK_SHARED);
         ret = iomap_dio_rw(iocb, to, &btrfs_dio_iomap_ops, &btrfs_dops, flags);
-	inode_unlock_shared(inode);
+	btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
+
 	return ret;
 }