| Message ID | 27a36e2b4cf30de8f7a04e718ba47bb9e98ddb85.1658419807.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix infinite loop with dio faulting | expand |
On Thu, Jul 21, 2022 at 12:10:14PM -0400, Josef Bacik wrote: > Filipe removed the check for the case where we are constantly trying to > fault in the buffer from user space for DIO reads. He left it for > writes, but for reads you can end up in this infinite loop trying to > fault in a page that won't fault in. This is the patch I applied ontop > of his patch, without this I was able to get generic/475 to get stuck > infinite looping. > > This patch is currently staged so we can probably just fold this into > his actual patch. Folded to the patch, thanks.
On Thu, Jul 21, 2022 at 5:12 PM Josef Bacik <josef@toxicpanda.com> wrote: > > Filipe removed the check for the case where we are constantly trying to > fault in the buffer from user space for DIO reads. He left it for > writes, but for reads you can end up in this infinite loop trying to > fault in a page that won't fault in. Yes, and the removal was intentional (not by accident). Adding the check back makes the same check at dio_fault_in_size() useless and redundant. > This is the patch I applied ontop > of his patch, without this I was able to get generic/475 to get stuck > infinite looping. This doesn't answer why the infinite loop happens. Why can't we faultin at least one page? Have you checked why this happens? So I'd rather not fold this into the original patch, and instead revert it until we know exactly why we end in the infinite loop. Thanks. > This patch is currently staged so we can probably just fold this into > his actual patch. > > Fixes: 5ad7531dbe67 ("btrfs: fault in pages for direct io reads/writes in a more controlled way") > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/file.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index 1876072dee9d..79f866e36368 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -3806,20 +3806,21 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) > read = ret; > > if (iov_iter_count(to) > 0 && (ret == -EFAULT || ret > 0)) { > - if (iter_is_iovec(to)) { > - const size_t left = iov_iter_count(to); > - const size_t size = dio_fault_in_size(iocb, to, prev_left); > + const size_t left = iov_iter_count(to); > > - fault_in_iov_iter_writeable(to, size); > - prev_left = left; > - goto again; > - } else { > + if (left == prev_left) { > /* > * fault_in_iov_iter_writeable() only works for iovecs, > * return with a partial read and fallback to buffered > * IO for the rest of the range. > */ > ret = read; > + } else { > + const size_t size = dio_fault_in_size(iocb, to, prev_left); > + > + fault_in_iov_iter_writeable(to, size); > + prev_left = left; > + goto again; > } > } > btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED); > -- > 2.26.3 >
On Tue, Aug 2, 2022 at 7:00 PM David Sterba <dsterba@suse.cz> wrote: > > On Thu, Jul 21, 2022 at 12:10:14PM -0400, Josef Bacik wrote: > > Filipe removed the check for the case where we are constantly trying to > > fault in the buffer from user space for DIO reads. He left it for > > writes, but for reads you can end up in this infinite loop trying to > > fault in a page that won't fault in. This is the patch I applied ontop > > of his patch, without this I was able to get generic/475 to get stuck > > infinite looping. > > > > This patch is currently staged so we can probably just fold this into > > his actual patch. > > Folded to the patch, thanks. Please don't, revert instead the original patch. See my previous reply to the patch. Thanks.
On Tue, Aug 02, 2022 at 07:09:28PM +0100, Filipe Manana wrote: > On Tue, Aug 2, 2022 at 7:00 PM David Sterba <dsterba@suse.cz> wrote: > > > > On Thu, Jul 21, 2022 at 12:10:14PM -0400, Josef Bacik wrote: > > > Filipe removed the check for the case where we are constantly trying to > > > fault in the buffer from user space for DIO reads. He left it for > > > writes, but for reads you can end up in this infinite loop trying to > > > fault in a page that won't fault in. This is the patch I applied ontop > > > of his patch, without this I was able to get generic/475 to get stuck > > > infinite looping. > > > > > > This patch is currently staged so we can probably just fold this into > > > his actual patch. > > > > Folded to the patch, thanks. > > Please don't, revert instead the original patch. OK, patch removed from misc-next, no need to revert.
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 1876072dee9d..79f866e36368 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -3806,20 +3806,21 @@ static ssize_t btrfs_direct_read(struct kiocb *iocb, struct iov_iter *to) read = ret; if (iov_iter_count(to) > 0 && (ret == -EFAULT || ret > 0)) { - if (iter_is_iovec(to)) { - const size_t left = iov_iter_count(to); - const size_t size = dio_fault_in_size(iocb, to, prev_left); + const size_t left = iov_iter_count(to); - fault_in_iov_iter_writeable(to, size); - prev_left = left; - goto again; - } else { + if (left == prev_left) { /* * fault_in_iov_iter_writeable() only works for iovecs, * return with a partial read and fallback to buffered * IO for the rest of the range. */ ret = read; + } else { + const size_t size = dio_fault_in_size(iocb, to, prev_left); + + fault_in_iov_iter_writeable(to, size); + prev_left = left; + goto again; } } btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
Filipe removed the check for the case where we are constantly trying to fault in the buffer from user space for DIO reads. He left it for writes, but for reads you can end up in this infinite loop trying to fault in a page that won't fault in. This is the patch I applied ontop of his patch, without this I was able to get generic/475 to get stuck infinite looping. This patch is currently staged so we can probably just fold this into his actual patch. Fixes: 5ad7531dbe67 ("btrfs: fault in pages for direct io reads/writes in a more controlled way") Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/file.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)