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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3D2E2C07E97 for ; Thu, 30 Nov 2023 02:25:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8CEAC8D002B; Wed, 29 Nov 2023 21:25:57 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 87F348D0001; Wed, 29 Nov 2023 21:25:57 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 747158D002B; Wed, 29 Nov 2023 21:25:57 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 64F0A8D0001 for ; Wed, 29 Nov 2023 21:25:57 -0500 (EST) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 47FC5406F7 for ; Thu, 30 Nov 2023 02:25:57 +0000 (UTC) X-FDA: 81513030354.15.5B537E6 Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) by imf13.hostedemail.com (Postfix) with ESMTP id 6856F2000F for ; Thu, 30 Nov 2023 02:25:55 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=RhQ2U4Ow; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of smfrench@gmail.com designates 209.85.167.46 as permitted sender) smtp.mailfrom=smfrench@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1701311155; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Y6Rdw8yGxfDCWZVL2kr+wdCOFUJXsgDKk095weKI06U=; b=IZTfVlW3pR5gzFmd2ZQe6ngUok54isLr6r8yJaD2z1a48wyk5HdniS/8gckNwrrFaxFs7n pC4Avs3IvQBNUwDbtD01CgbM5L+/Sk8VPP3GcJz7vK4nIJ94+MODGvq40DssPS/O3VHgwN GcbWBvqSAvMwWvzZf9FsW8mIJjgTcws= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=RhQ2U4Ow; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of smfrench@gmail.com designates 209.85.167.46 as permitted sender) smtp.mailfrom=smfrench@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701311155; a=rsa-sha256; cv=none; b=2HxKhds88VXpuVYNwk7fRwMzwyzvCTNsmZU4vFejzOPNX7jQ1GfOcVXBmIci+WkSXP5adK 3ycPqcCXHvigJPGR4mQzOtT+QVRNR8/n4HDPanTiDyHVP6ETMX95vfN2GbAPv1Hh9n+GaV 5hitWZ+Z7ASywfhfIEJe4vZ4QWVfPpU= Received: by mail-lf1-f46.google.com with SMTP id 2adb3069b0e04-50bb92811c0so425761e87.1 for ; Wed, 29 Nov 2023 18:25:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701311153; x=1701915953; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Y6Rdw8yGxfDCWZVL2kr+wdCOFUJXsgDKk095weKI06U=; b=RhQ2U4OwnigsAPkRa/N5eK4YW/2ijYjF3KSkbKZRoi8eS8c5V2tyqWoec9a2bnDWcD 5MrP5+RVACBdLMJMUe2obhQJHz90r1o35/KWYmW0ORvkqbZpPJEUOtjkQDlpklZRR1KM 435jVbHuHXIzfDM3IwaqAnSx/Fapwq07Wa8Fjes+zE4nunQFqKwqG3rgzRTiHE6iCsMK Ws+mDw5Rvo6s2cI8nZTpa6mV6bYj4EwYLgTEu9XtGNRTq2pzrS3ODwySSChRlRQ83Vro 8xN5juJohTvFPHKiw+5lpyiO6+PIwd6YkC9HuCdHj/IFhOhsJBOIEwv2aw6k29fDE6IS g7Iw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701311153; x=1701915953; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Y6Rdw8yGxfDCWZVL2kr+wdCOFUJXsgDKk095weKI06U=; b=HW3uCK6Z5ZQLGukF4YM7EgQdDBsZ3QyN9ejMqHhiweCK2rfBzPA6ZlEEiZEcqtyzQr lrmeRsLOaxIABr37Zd+6KPpfl/hV4vwXmRzKSpxokpyIrG2X/N5evVuhLOIt1QxM3vli MR2cwpAWpqbgrtHCp9lmeoHdRglMdn4+VbjemkTzNZk8Ra5NaqdTge9DiFxqv5CFRtdD YOSx8ou7b7RYOVdpg0TLr9vjPhmMJtpl+cGi18EUGPCX3qbaWv/fKUrq+NWyGXvEPCxT +07IcG3VhCHO+JZX++NgJTkeDMOEe3fokH0rD47AriLI/59bANP+cZp/Kw+bIPndUDIl r8ZA== X-Gm-Message-State: AOJu0YwAEWTco1jW2j0zekPY/BR4dOV0aDrbWMDYNJ6HhyPr9SKu7gDu AbrYJGDM6kgR00YArDIqLhO3QEtONHXi3pVtCLQ= X-Google-Smtp-Source: AGHT+IHkwlTtGobFBVK9Zh7Psmm/BWGm9Q7o9I7hI4ZQ90TxYhWrksx8rgl2XRMyW7v7JpZtxOdiArv5r1Nit8rujTM= X-Received: by 2002:ac2:4826:0:b0:50a:7552:15e9 with SMTP id 6-20020ac24826000000b0050a755215e9mr6536736lft.21.1701311153280; Wed, 29 Nov 2023 18:25:53 -0800 (PST) MIME-Version: 1.0 References: <20231129165619.2339490-1-dhowells@redhat.com> <20231129165619.2339490-4-dhowells@redhat.com> <9704ab96ba04eb3591a62ef5e6a97af6@manguebit.com> In-Reply-To: <9704ab96ba04eb3591a62ef5e6a97af6@manguebit.com> From: Steve French Date: Wed, 29 Nov 2023 20:25:42 -0600 Message-ID: Subject: Re: [PATCH 3/3] cifs: Fix flushing, invalidation and file size with copy_file_range() To: Paulo Alcantara Cc: David Howells , Steve French , Shyam Prasad N , Rohith Surabattula , Matthew Wilcox , Jeff Layton , linux-cifs@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org Content-Type: multipart/alternative; boundary="000000000000f19bec060b555e62" X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 6856F2000F X-Stat-Signature: jny6rjde1ipk9ho4zc56fjqe9nbpcidq X-Rspam-User: X-HE-Tag: 1701311155-121967 X-HE-Meta: U2FsdGVkX1+9tR5gco4pDtqWQMBkT6fvG4hZtut9vEC/C+JiXCVutS+vL7rA+9K+rR4EchT9NsrH91AY+6xS+7ZJOEG7NX4gjg51loUwfc+8ns5UWFAXuK4UhfkJKDnMk+P9B5u5/aYMWkcO1F8FwMBzxfdHDyzDZHeB9y2IKXF5bkxEg4tPLRNVl+WG85bWAwhg+EmNzFSXRS9NCwJbfMvgIIGdOelG2Jwbga8Ok5UHS7vkLxEfvRkWouYEpai1PGy+nWT43we5Y4qJzs1mbCBBjmyehJggGELdywQXSXeAvTLckOv/Oth8j3EHtzf1nafO52nhhC5el1kaeiLclAu8NGmj7d0eAZWSxstduZbLJJFIPhhPAqJX0pUfg9PrTeBUPBKSihxkEco8LmqYiEnq2FpibKm70Hdy7Yk8GNURFMAdHsCWZAhORLD6C+NBy1NWxt/d/I/rfr6rbPJ5+tFlrmE4tsFg0oPMegSzxZAJgZDK63bS9vLqWP/U+ctPq5rWgla2OTPHo5hJ/yUYiDQ4nPv7Gbt9x9ssd8YRpHNMdArFu1aI/RH0jWCd5T7/8gzTn2yF9l84+164QFrfTwo+//b4nsa+l9QQ84s3aWrVKO9Ppfp0D8YTBTxJVP87tLT8ogK8e0NqX3TMGKyBXxzUEqGvlM1uyNaQlyUyiqivez365ksJx62AcfvRm1b8pCyAeyufBtQnfkb0rsJj5vOP4gIai2/IhTAqUHveOv/rrhcuwz915w3T3kmN7WztQdfutL2VpwXak3Z6a4mk0rxkVhT4dNPIz+hqlgO6gGaL/JxohsSZ7F+fg5N5U/zniI6avDwmtnnjmGaFcGYgR9lKNQMmrp3vd98iLU+38hqJzWgxynyV6sh2H/l4JOl88GiehQEd0PH945joGIpJwAnxQLd/dfZxnTOHa/oCLeltSCSMzWxpqs7BugfbZF+jlBCCJVvpP+EoraolWxu 46JIiFQT IKmEHluxo9eV3pHrwf8WZLaHXzYlkWxP/DXJfVcft4xExqaorGPkhRbkVVNCoWqDN7d6FJjhEaXM+AVmD+Docl4GwxUE3qgzb5EvWjf9bezdcTFjZA6MDieN7SJTxF6Bm9yP+uj/u+gzm0/NGdoPmQXnYD03763q59kEbM3To3PVvJzA015GD8jZmk3DY/kQeoi6FjOgV8Rts+jT4NNnHBEsBDOxelt1SQKj0Nfp/yUxfuS02hKFroPvdyqaWkDu9OeN78AI0AwJRxW2NMStoGwOv0txAy+hJm4JXOmUaqbNiJAQULB2+ohrldZCO+ye85SpIJG3p9mXnzZeJoGjEcp47bXVTf2j9VS+RadfHz0LXNCVN53Japqa6nSzPaPSwkuICpN/Gc9GGVcRJV3OxZLhmAZXs3n+PCbTsao6A8LuIhSvSmtxDWmO8XjHCoUW+X41KyiX3cMMqtJ/5//jychu8X+1zzjGnC7Ac442509Lhj0NL48LkhyfGUam2urVTP1pEtqf0K41/RrCBmsY6R6j3/6kaZ6EqYzpqf0N2fBpMf+sKnjJwJt25uf4mAahbkL3Zr9pJxSucfyeXkbnuKSD2iDmDSwZARZaxDw34Qb5PmJICRqzsYMe+FHj2TNcnC+xMDT1Kvrg0V6GmKqMbtaZRWuxqkCVN/rL07v2DMhIT+iNPFUsayDnMJL4SeQ0z9ZOWPR4tvmCjw/OQ+iMo0nCJzv044nfcRf/LzUFG/boeCdhRYDefGdvwoysr9Pv1XTog24lck+b8zyq2WlHUTeYhmwBkPvOjD/MD81MsGyhPKp6jUEKaTK4fokfrLlf6SU8w6IJURFRkba+fX8VFRESOGql+qZ1EinAB/gp4nzH2TDmdbPJdA9LNa92gJPLN1GnwiHzfVPQWmFGCDrJ44+7ISfE6EToLw2Ko X-Bogosity: Ham, tests=bogofilter, spamicity=0.000640, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: --000000000000f19bec060b555e62 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable updated all three patches with the acked-by and put in cifs-2.6.git for-nex= t On Wed, Nov 29, 2023 at 4:28=E2=80=AFPM Paulo Alcantara = wrote: > David Howells writes: > > > Fix a number of issues in the cifs filesystem implementation of the > > copy_file_range() syscall in cifs_file_copychunk_range(). > > > > Firstly, the invalidation of the destination range is handled > incorrectly: > > We shouldn't just invalidate the whole file as dirty data in the file m= ay > > get lost and we can't just call truncate_inode_pages_range() to > invalidate > > the destination range as that will erase parts of a partial folio at ea= ch > > end whilst invalidating and discarding all the folios in the middle. W= e > > need to force all the folios covering the range to be reloaded, but we > > mustn't lose dirty data in them that's not in the destination range. > > > > Further, we shouldn't simply round out the range to PAGE_SIZE at each e= nd > > as cifs should move to support multipage folios. > > > > Secondly, there's an issue whereby a write may have extended the file > > locally, but not have been written back yet. This can leaves the local > > idea of the EOF at a later point than the server's EOF. If a copy > request > > is issued, this will fail on the server with STATUS_INVALID_VIEW_SIZE > > (which gets translated to -EIO locally) if the copy source extends past > the > > server's EOF. > > > > Fix this by: > > > > (0) Flush the source region (already done). The flush does nothing an= d > > the EOF isn't moved if the source region has no dirty data. > > > > (1) Move the EOF to the end of the source region if it isn't already a= t > > least at this point. > > > > [!] Rather than moving the EOF, it might be better to split the co= py > > range into a part to be copied and a part to be cleared with > > FSCTL_SET_ZERO_DATA. > > > > (2) Find the folio (if present) at each end of the range, flushing it > and > > increasing the region-to-be-invalidated to cover those in their > > entirety. > > > > (3) Fully discard all the folios covering the range as we want them to > be > > reloaded. > > > > (4) Then perform the copy. > > > > Thirdly, set i_size after doing the copychunk_range operation as this > value > > may be used by various things internally. stat() hides the issue becau= se > > setting ->time to 0 causes cifs_getatr() to revalidate the attributes. > > > > These were causing the generic/075 xfstest to fail. > > > > Fixes: 620d8745b35d ("Introduce cifs_copy_file_range()") > > Signed-off-by: David Howells > > cc: Steve French > > cc: Paulo Alcantara > > cc: Shyam Prasad N > > cc: Rohith Surabattula > > cc: Matthew Wilcox > > cc: Jeff Layton > > cc: linux-cifs@vger.kernel.org > > cc: linux-mm@kvack.org > > --- > > fs/smb/client/cifsfs.c | 80 ++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 77 insertions(+), 3 deletions(-) > > Looks good, > > Acked-by: Paulo Alcantara (SUSE) > > --=20 Thanks, Steve --000000000000f19bec060b555e62 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
updated all three patches with the acked-by and put in cif= s-2.6.git for-next

On Wed, Nov 29, 2023 at 4:28=E2=80=AFPM Paulo Alcantara &= lt;pc@manguebit.com> wrote:
<= /div>
David Howells <dhowells@redhat.com<= /a>> writes:

> Fix a number of issues in the cifs filesystem implementation of the > copy_file_range() syscall in cifs_file_copychunk_range().
>
> Firstly, the invalidation of the destination range is handled incorrec= tly:
> We shouldn't just invalidate the whole file as dirty data in the f= ile may
> get lost and we can't just call truncate_inode_pages_range() to in= validate
> the destination range as that will erase parts of a partial folio at e= ach
> end whilst invalidating and discarding all the folios in the middle.= =C2=A0 We
> need to force all the folios covering the range to be reloaded, but we=
> mustn't lose dirty data in them that's not in the destination = range.
>
> Further, we shouldn't simply round out the range to PAGE_SIZE at e= ach end
> as cifs should move to support multipage folios.
>
> Secondly, there's an issue whereby a write may have extended the f= ile
> locally, but not have been written back yet.=C2=A0 This can leaves the= local
> idea of the EOF at a later point than the server's EOF.=C2=A0 If a= copy request
> is issued, this will fail on the server with STATUS_INVALID_VIEW_SIZE<= br> > (which gets translated to -EIO locally) if the copy source extends pas= t the
> server's EOF.
>
> Fix this by:
>
>=C2=A0 (0) Flush the source region (already done).=C2=A0 The flush does= nothing and
>=C2=A0 =C2=A0 =C2=A0 the EOF isn't moved if the source region has n= o dirty data.
>
>=C2=A0 (1) Move the EOF to the end of the source region if it isn't= already at
>=C2=A0 =C2=A0 =C2=A0 least at this point.
>
>=C2=A0 =C2=A0 =C2=A0 [!] Rather than moving the EOF, it might be better= to split the copy
>=C2=A0 =C2=A0 =C2=A0 range into a part to be copied and a part to be cl= eared with
>=C2=A0 =C2=A0 =C2=A0 FSCTL_SET_ZERO_DATA.
>
>=C2=A0 (2) Find the folio (if present) at each end of the range, flushi= ng it and
>=C2=A0 =C2=A0 =C2=A0 increasing the region-to-be-invalidated to cover t= hose in their
>=C2=A0 =C2=A0 =C2=A0 entirety.
>
>=C2=A0 (3) Fully discard all the folios covering the range as we want t= hem to be
>=C2=A0 =C2=A0 =C2=A0 reloaded.
>
>=C2=A0 (4) Then perform the copy.
>
> Thirdly, set i_size after doing the copychunk_range operation as this = value
> may be used by various things internally.=C2=A0 stat() hides the issue= because
> setting ->time to 0 causes cifs_getatr() to revalidate the attribut= es.
>
> These were causing the generic/075 xfstest to fail.
>
> Fixes: 620d8745b35d ("Introduce cifs_copy_file_range()")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Steve French <sfrench@samba.org>
> cc: Paulo Alcantara <pc@manguebit.com>
> cc: Shyam Prasad N <nspmangalore@gmail.com>
> cc: Rohith Surabattula <rohiths.msft@gmail.com>
> cc: Matthew Wilcox <willy@infradead.org>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: li= nux-cifs@vger.kernel.org
> cc: linux-mm@k= vack.org
> ---
>=C2=A0 fs/smb/client/cifsfs.c | 80 ++++++++++++++++++++++++++++++++++++= ++++--
>=C2=A0 1 file changed, 77 insertions(+), 3 deletions(-)

Looks good,

Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com>



--
Th= anks,

Steve
--000000000000f19bec060b555e62--