From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A071FC04EB8 for ; Tue, 4 Dec 2018 21:35:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 697E020672 for ; Tue, 4 Dec 2018 21:35:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 697E020672 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=fromorbit.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-nfs-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725897AbeLDVfx (ORCPT ); Tue, 4 Dec 2018 16:35:53 -0500 Received: from ipmail03.adl2.internode.on.net ([150.101.137.141]:20643 "EHLO ipmail03.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725887AbeLDVfx (ORCPT ); Tue, 4 Dec 2018 16:35:53 -0500 Received: from ppp59-167-129-252.static.internode.on.net (HELO dastard) ([59.167.129.252]) by ipmail03.adl2.internode.on.net with ESMTP; 05 Dec 2018 07:59:50 +1030 Received: from dave by dastard with local (Exim 4.80) (envelope-from ) id 1gUIGK-0005pb-Q6; Wed, 05 Dec 2018 08:29:48 +1100 Date: Wed, 5 Dec 2018 08:29:48 +1100 From: Dave Chinner To: Christoph Hellwig Cc: Amir Goldstein , linux-fsdevel , linux-xfs , Olga Kornievskaia , Linux NFS Mailing List , overlayfs , ceph-devel@vger.kernel.org, linux-cifs@vger.kernel.org Subject: Re: [PATCH 01/11] vfs: copy_file_range source range over EOF should fail Message-ID: <20181204212948.GO6311@dastard> References: <20181203083416.28978-1-david@fromorbit.com> <20181203083416.28978-2-david@fromorbit.com> <20181204151332.GA32245@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181204151332.GA32245@infradead.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org On Tue, Dec 04, 2018 at 07:13:32AM -0800, Christoph Hellwig wrote: > On Mon, Dec 03, 2018 at 02:46:20PM +0200, Amir Goldstein wrote: > > > From: Dave Chinner > > > > > > The man page says: > > > > > > EINVAL Requested range extends beyond the end of the source file > > > > > > But the current behaviour is that copy_file_range does a short > > > copy up to the source file EOF. Fix the kernel behaviour to match > > > the behaviour described in the man page. > > I think the behavior implemented is a lot more useful than the one > documented.. The current behaviour is really nasty. Because copy_file_range() can return short copies, the caller has to implement a loop to ensure the range hey want get copied. When the source range you are trying to copy overlaps source EOF, this loop: while (len > 0) { ret = copy_file_range(... len ...) ... off_in += ret; off_out += ret; len -= ret; } Currently the fallback code copies up to the end of the source file on the first copy and then fails the second copy with EINVAL because the source range is now completely beyond EOF. So, from an application perspective, did the copy succeed or did it fail? Existing tools that exercise copy_file_range (like xfs_io) consider this a failure, because the second copy_file_range() call returns EINVAL and not some "there is no more to copy" marker like read() returning 0 bytes when attempting to read beyond EOF. IOWs, we cannot tell the difference between a real error and a short copy because the input range spans EOF and it was silently shortened. That's the API problem we need to fix here - the existing behaviour is really crappy for applications. Erroring out immmediately is one solution, and it's what the man page says should happen so that is what I implemented. Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though. > > i_size_read()... > > > > Otherwise > > Reviewed-by: Amir Goldstein > > Looks like this doesn't even compile? It's fixed in a later patch that consolidates the checks into a generic check function, but I'm not sure why my "compile every patch" script didn't catch this. Cheers, -Dave. -- Dave Chinner david@fromorbit.com