Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1424391AbdDUTC3 (ORCPT ); Fri, 21 Apr 2017 15:02:29 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:42324 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1424352AbdDUTCZ (ORCPT ); Fri, 21 Apr 2017 15:02:25 -0400 Date: Fri, 21 Apr 2017 18:54:30 +0100 From: Al Viro To: Dave Jones , Linux Kernel Subject: Re: iov_iter_pipe warning. Message-ID: <20170421175430.GT29622@ZenIV.linux.org.uk> References: <20170412001746.GM29622@ZenIV.linux.org.uk> <20170412005853.vqyuo6722tmthn5u@codemonkey.org.uk> <20170412011532.GN29622@ZenIV.linux.org.uk> <20170412022911.nhefjqlnyrk3n7rr@codemonkey.org.uk> <20170412025842.GO29622@ZenIV.linux.org.uk> <20170412143519.4hh36l3egozgdrll@codemonkey.org.uk> <20170412152600.GP29622@ZenIV.linux.org.uk> <20170412162709.bn5qfk4seues3yos@codemonkey.org.uk> <20170412170723.GQ29622@ZenIV.linux.org.uk> <20170412190318.srkkdytf2ebrjzrg@codemonkey.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170412190318.srkkdytf2ebrjzrg@codemonkey.org.uk> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4204 Lines: 121 On Wed, Apr 12, 2017 at 03:03:18PM -0400, Dave Jones wrote: > Well it's been running an hour without incident, which looks promising. > I'll leave it run, but I'd say you're on the right track given how quick > it reproduced so far. Could you try this and see if it works? What happens is that unlike e.g. generic_file_read_iter/generic_file_write_iter, NFS O_DIRECT handling does not make sure that iov_iter had been advanced by the amount actually transferred - it is left advanced by the amount *requested*. mm/filemap.c code gets around that by taking a copy of iov_iter, feeding it to ->direct_IO() and then advancing the original by the amount actually done. That's what the previous patch had duplicated for NFS, but we have a cleaner way to do that now - both for NFS and in mm/filemap.c. Namely, use iov_iter_revert(). For NFS it means making nfs_direct_..._schedule_iovec() return how much it has actually requested from server and having their callers do iov_iter_revert() after nfs_direct_wait() has reported how much has actually come through. I've similar patches for mm/filemap.c avoiding the games with copy of iov_iter there, but those are not fixes per se, so they are separate. This one just deals with NFS. fix nfs O_DIRECT advancing iov_iter too much It leaves the iterator advanced by the amount of IO it has requested instead of the amount actually transferred. Among other things, that confuses the hell out of generic_file_splice_read(). Signed-off-by: Al Viro diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c index aab32fc3d6a8..c1b5fed7c863 100644 --- a/fs/nfs/direct.c +++ b/fs/nfs/direct.c @@ -537,7 +537,7 @@ static ssize_t nfs_direct_read_schedule_iovec(struct nfs_direct_req *dreq, if (put_dreq(dreq)) nfs_direct_complete(dreq); - return 0; + return requested_bytes; } /** @@ -566,7 +566,7 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, struct iov_iter *iter) struct inode *inode = mapping->host; struct nfs_direct_req *dreq; struct nfs_lock_context *l_ctx; - ssize_t result = -EINVAL; + ssize_t result = -EINVAL, requested; size_t count = iov_iter_count(iter); nfs_add_stats(mapping->host, NFSIOS_DIRECTREADBYTES, count); @@ -600,14 +600,19 @@ ssize_t nfs_file_direct_read(struct kiocb *iocb, struct iov_iter *iter) nfs_start_io_direct(inode); NFS_I(inode)->read_io += count; - result = nfs_direct_read_schedule_iovec(dreq, iter, iocb->ki_pos); + requested = nfs_direct_read_schedule_iovec(dreq, iter, iocb->ki_pos); nfs_end_io_direct(inode); - if (!result) { + if (requested > 0) { result = nfs_direct_wait(dreq); - if (result > 0) + if (result > 0) { + requested -= result; iocb->ki_pos += result; + } + iov_iter_revert(iter, requested); + } else { + result = requested; } out_release: @@ -954,7 +959,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq, if (put_dreq(dreq)) nfs_direct_write_complete(dreq); - return 0; + return requested_bytes; } /** @@ -979,7 +984,7 @@ static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq, */ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter) { - ssize_t result = -EINVAL; + ssize_t result = -EINVAL, requested; size_t count; struct file *file = iocb->ki_filp; struct address_space *mapping = file->f_mapping; @@ -1022,7 +1027,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter) nfs_start_io_direct(inode); - result = nfs_direct_write_schedule_iovec(dreq, iter, pos); + requested = nfs_direct_write_schedule_iovec(dreq, iter, pos); if (mapping->nrpages) { invalidate_inode_pages2_range(mapping, @@ -1031,13 +1036,17 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter) nfs_end_io_direct(inode); - if (!result) { + if (requested > 0) { result = nfs_direct_wait(dreq); if (result > 0) { + requested -= result; iocb->ki_pos = pos + result; /* XXX: should check the generic_write_sync retval */ generic_write_sync(iocb, result); } + iov_iter_revert(iter, requested); + } else { + result = requested; } out_release: nfs_direct_req_release(dreq);