| Message ID | 5ee370a435eb08fb14579c7c197b16e9fa0886f0.1571647179.git.mbobrowski@mbobrowski.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: port direct I/O to iomap infrastructure | expand |
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); > /*
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>--
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
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); > /* >
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); /*
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(-)