[v5,08/12] ext4: update direct I/O read to do trylock in IOCB_NOWAIT cases
diff mbox series

Message ID 5ee370a435eb08fb14579c7c197b16e9fa0886f0.1571647179.git.mbobrowski@mbobrowski.org
State Superseded
Headers show
Series
  • ext4: port direct I/O to iomap infrastructure
Related show

Commit Message

Matthew Bobrowski Oct. 21, 2019, 9:18 a.m. UTC
This patch updates the lock pattern in ext4_dio_read_iter() to only
perform the trylock in IOCB_NOWAIT cases.

Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
---
 fs/ext4/file.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Jan Kara Oct. 21, 2019, 1:48 p.m. UTC | #1
On Mon 21-10-19 20:18:46, Matthew Bobrowski wrote:
> This patch updates the lock pattern in ext4_dio_read_iter() to only
> perform the trylock in IOCB_NOWAIT cases.

The changelog is actually misleading. It should say something like "This
patch updates the lock pattern in ext4_dio_read_iter() to not block on
inode lock in case of IOCB_NOWAIT direct IO reads."

Also to ease backporting of easy fixes, we try to put patches like this
early in the series (fixing code in ext4_direct_IO_read(), and then the
fixed code would just carry over to ext4_dio_read_iter()).

The change itself looks good to me.

								Honza

> 
> Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> ---
>  fs/ext4/file.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 6ea7e00e0204..8420686b90f5 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -52,7 +52,13 @@ static int ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  	ssize_t ret;
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  
> -	inode_lock_shared(inode);
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock_shared(inode))
> +			return -EAGAIN;
> +	} else {
> +		inode_lock_shared(inode);
> +	}
> +
>  	if (!ext4_dio_supported(inode)) {
>  		inode_unlock_shared(inode);
>  		/*
Matthew Bobrowski Oct. 22, 2019, 2:04 a.m. UTC | #2
On Mon, Oct 21, 2019 at 03:48:17PM +0200, Jan Kara wrote:
> On Mon 21-10-19 20:18:46, Matthew Bobrowski wrote:
> > This patch updates the lock pattern in ext4_dio_read_iter() to only
> > perform the trylock in IOCB_NOWAIT cases.
> 
> The changelog is actually misleading. It should say something like "This
> patch updates the lock pattern in ext4_dio_read_iter() to not block on
> inode lock in case of IOCB_NOWAIT direct IO reads."
> 
> Also to ease backporting of easy fixes, we try to put patches like this
> early in the series (fixing code in ext4_direct_IO_read(), and then the
> fixed code would just carry over to ext4_dio_read_iter()).

OK, understood. Now I know this for next time. :)

Providing that I have this patch precede the ext4_dio_read_iter()
patch and implement this lock pattern in ext4_direct_IO_read(), am I
OK to add the 'Reviewed-by' tag?

--<M>--
Jan Kara Oct. 22, 2019, 7:50 a.m. UTC | #3
On Tue 22-10-19 13:04:21, Matthew Bobrowski wrote:
> On Mon, Oct 21, 2019 at 03:48:17PM +0200, Jan Kara wrote:
> > On Mon 21-10-19 20:18:46, Matthew Bobrowski wrote:
> > > This patch updates the lock pattern in ext4_dio_read_iter() to only
> > > perform the trylock in IOCB_NOWAIT cases.
> > 
> > The changelog is actually misleading. It should say something like "This
> > patch updates the lock pattern in ext4_dio_read_iter() to not block on
> > inode lock in case of IOCB_NOWAIT direct IO reads."
> > 
> > Also to ease backporting of easy fixes, we try to put patches like this
> > early in the series (fixing code in ext4_direct_IO_read(), and then the
> > fixed code would just carry over to ext4_dio_read_iter()).
> 
> OK, understood. Now I know this for next time. :)
> 
> Providing that I have this patch precede the ext4_dio_read_iter()
> patch and implement this lock pattern in ext4_direct_IO_read(), am I
> OK to add the 'Reviewed-by' tag?

Yes.

								Honza
Ritesh Harjani Oct. 23, 2019, 6:51 a.m. UTC | #4
On 10/21/19 2:48 PM, Matthew Bobrowski wrote:
> This patch updates the lock pattern in ext4_dio_read_iter() to only
> perform the trylock in IOCB_NOWAIT cases.
> 
> Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>

This shall need a rebase (as mentioned in patch-1 discussion).
It will be good to mention that the locking condition
is kept similar to 942491c9e6d6 ("xfs: fix AIM7 regression").

Also, I think it may also need a fixes tag.
It seems unconditional inode_lock_shared was added by below commit.

Fixes: 16c54688592c ("ext4: Allow parallel DIO reads")


Otherwise, this patch looks good to me. You may add:

Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>


> ---
>   fs/ext4/file.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 6ea7e00e0204..8420686b90f5 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -52,7 +52,13 @@ static int ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
>   	ssize_t ret;
>   	struct inode *inode = file_inode(iocb->ki_filp);
>   
> -	inode_lock_shared(inode);
> +	if (iocb->ki_flags & IOCB_NOWAIT) {
> +		if (!inode_trylock_shared(inode))
> +			return -EAGAIN;
> +	} else {
> +		inode_lock_shared(inode);
> +	}
> +
>   	if (!ext4_dio_supported(inode)) {
>   		inode_unlock_shared(inode);
>   		/*
>

Patch
diff mbox series

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 6ea7e00e0204..8420686b90f5 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -52,7 +52,13 @@  static int ext4_dio_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	ssize_t ret;
 	struct inode *inode = file_inode(iocb->ki_filp);
 
-	inode_lock_shared(inode);
+	if (iocb->ki_flags & IOCB_NOWAIT) {
+		if (!inode_trylock_shared(inode))
+			return -EAGAIN;
+	} else {
+		inode_lock_shared(inode);
+	}
+
 	if (!ext4_dio_supported(inode)) {
 		inode_unlock_shared(inode);
 		/*