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 504FAC4167B for ; Fri, 1 Dec 2023 00:50:05 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B5A396B0461; Thu, 30 Nov 2023 19:50:04 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B0A596B0463; Thu, 30 Nov 2023 19:50:04 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 983D26B046F; Thu, 30 Nov 2023 19:50:04 -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 854A06B0461 for ; Thu, 30 Nov 2023 19:50:04 -0500 (EST) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id D9D0C1201A5 for ; Fri, 1 Dec 2023 00:50:03 +0000 (UTC) X-FDA: 81516417486.10.66EE165 Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) by imf02.hostedemail.com (Postfix) with ESMTP id EC5598000C for ; Fri, 1 Dec 2023 00:50:01 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ffHp07On; spf=pass (imf02.hostedemail.com: domain of smfrench@gmail.com designates 209.85.167.52 as permitted sender) smtp.mailfrom=smfrench@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1701391802; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=XqzRhhGrZoC77SS6CL7pdFs8CHWp/wWWuAFxkrL+mk4=; b=Bugu2J6R8tLhYa1Q3lzGv8fWMnmV0FN1zC0xDRc18osWdx9u2hkTePUjeFzB13XrvheFE2 3JpBVLugAnMrSa4nA4xlpmvLB/gqLgog9sprshVw8JJNfAsDdj7iW/1GxWvM+1i8iM4wRp Cxj3bXoLsBtM9PV9WzR3pAxICCAjFb4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701391802; a=rsa-sha256; cv=none; b=8BdHDPTlje/wKQagKUPpROQB7CmXia48LDWjN1lYQAXbkJesMxRABupVKYV3IaShXzADIX R7028ZxyJiwebyJN3SJ7g3cRi/6Jyz3azECQLKi9VlFDnHiiKEPxgJxl2Ywct+sPoYC+QT h67BCMxz2CE5u8QhXckeragHKENhgpI= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ffHp07On; spf=pass (imf02.hostedemail.com: domain of smfrench@gmail.com designates 209.85.167.52 as permitted sender) smtp.mailfrom=smfrench@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-lf1-f52.google.com with SMTP id 2adb3069b0e04-50bc57d81f4so2297794e87.2 for ; Thu, 30 Nov 2023 16:50:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701391800; x=1701996600; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=XqzRhhGrZoC77SS6CL7pdFs8CHWp/wWWuAFxkrL+mk4=; b=ffHp07OnKlDMkdoUuaz9vGp7pol6xJofWGaDv9xNnhCS9sWUuUyGv8XxWImgt9K0Jb S+7dV+SkUWvo8QCmKLSNMpFKUHBMNfQbWDQ6guTAfLxfSGCuwQAF8DQEpHOm4FeUV9A0 e9y7YX1Us9vMCW/izJGK0pjEsz99D11v/0z03EJv5n2R9tgPA/zBtk8oydaVYVprzTS4 XiBRDHYpfW0UMLmSMnltWiEP3Z22FBzbi8DXQSefJjA6eTTB+PvuEG1UvcKHNiAmEsLO MjqJ81IleKvktwSgiIw/b8wP0KCA/P909iKmHsyBiF1xz0+5Ir/UP8jW9g5xmOes0GB4 7j3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701391800; x=1701996600; h=content-transfer-encoding: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=XqzRhhGrZoC77SS6CL7pdFs8CHWp/wWWuAFxkrL+mk4=; b=KnlKXGVAHHzadR/hCVU6/32ThC8NRksi9+NAzNGxDdUYCoQU9soQftwIo0GxYt7yjH EQ2Cwam9iy3kPWd2oDvtd/ezctiIcQzQfu3nIL+Q5xsFIq3oae4IkPmvP8eIbFqf2H6s hKxYeAbqIdGImlAZaAmwoyfw/vMv4f7J9Kdmz0T+avc2s5Fhu+IncQoUMPH/yB1YHLd0 jbrxlS3FWn6p6MTzglejaBC3w06zU5rbw5KQyngu8+rMVntCBma3QoSayrayoXhOUrSa oPqgvkhTgfxVfxMTlCA+Gfxj5JK2I/iRDmzGgmjF40G2s579I75VhIPSJvpNSfpnwrOD c7eg== X-Gm-Message-State: AOJu0YzQWSwd5Dx0T/S1Y5toMFs+Luqvq32SR2oaIijEyflDSQA4baHf I2o/D7sK4DAruMMrLj/z0qJYktLIf50BdPULnSO/FS6on9Y= X-Google-Smtp-Source: AGHT+IGkaaqQCCGExitNdERhjSlPmkgnOyZ+fu0oCv4R1hJOoipxICR+Px6hgYy1FF10R3vkHk4hg9Czpa2BWVBmTeE= X-Received: by 2002:ac2:59dc:0:b0:50b:d764:2919 with SMTP id x28-20020ac259dc000000b0050bd7642919mr133143lfn.177.1701391799795; Thu, 30 Nov 2023 16:49:59 -0800 (PST) MIME-Version: 1.0 References: <20231201002201.2981258-1-dhowells@redhat.com> <20231201002201.2981258-3-dhowells@redhat.com> In-Reply-To: <20231201002201.2981258-3-dhowells@redhat.com> From: Steve French Date: Thu, 30 Nov 2023 18:49:48 -0600 Message-ID: Subject: Re: [PATCH 2/2] cifs: Fix flushing, invalidation and file size with FICLONE To: David Howells Cc: Steve French , Paulo Alcantara , Shyam Prasad N , Rohith Surabattula , Matthew Wilcox , Jeff Layton , linux-cifs@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, Christoph Hellwig Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: EC5598000C X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: yudk44paf7taa7bdjmtjcb8iyuthsw6x X-HE-Tag: 1701391801-146792 X-HE-Meta: U2FsdGVkX1/TU/+Sao4PtlmXnWp8pYXvmaFPOt5v2hC9zrAScMwDwJAb7TIJb1rWaxYAtbrVtIJvOC450tZOHqGlJM3uyve3Uy++k9lx49yVQ6ZRsPeqyo323G4wIf3Qe89V9F5pmkIxP98QZeVExsxso30JYlX2x6pXclCILtJFystA3G7s6QQyuKqcpEG7SxOwZl7/mz3Cy4psJcz/4FHLyvEI3GQV3SjoybPdPF7DnGEDcmJUrzUs1LPsA72If9uOPenvwWz2cxi/Xa2U6KASAhplQQEv1oc1LGIRE6itubltIZvFXX0V4/El6NcbQ4db4QewNMda0KmufuJnXMKCD5+/tAE2Rr2fwNeWg7jPeXSHGJE4lJc1A+Lcf6GC/gkYxHp8li9utYYefNJ61UjC2X45CutVA5sI+atN5n+onrJIdLRGuH4b/Fw5ukEa83rtq4InPA8zdSkFwOSqsYmbJzMuJ8rYhttwY39kY+a7oBhQToopvEhXJOsZyllkjsYIdcPJaKu/nUa0vSKzBYUTj9B3nciixopzh/RSCU/9Vd2SPUeZhj95lySVLgm7q+cPABFYsugU+hy4VGhf/dJA/V4igD/RlHEo6t/+TF2+mQeIDGwZ+UqSdMpm3Rqzv7dGjjfBkw2MddnYWy/3UiLxCLV7sVOga4OWci3AvReLusb3MmB0ZDqhU/5/vr7Us2GgSqkvsQemJ+AhUdxV9Nk/wiQDXTPXsx56/jSy6PSVsl3S5ZGcpgOiorlnjy0+6g+AetVt0/vZSsVbbuETkD96j2bsy2BN4uMgu4BakSY0WZsdnzFWrBpWqKGsfEwFIu/tZDmNWMdVUtkrKYs8ou4nBRpjoZGah6r98U/UM2ysxqtozn5DurnEpY9xBj9322XopaKwz1sClHo0+w1ZxkXimrEUd2GVKgIKCct6+eG8zirR1l0I76rQ/LE1SXQxuXV1nUDZI9+BBVPL9cm Kerol0sI cO/0zjd/kBAEfuk2/RRc1iK/HU5yG9++xCbn4n86t38HvD++gW/ORNldKXDoWtYkuHEzwgsvfdZitwsfQ169HUQ+nN6mb6KS11sQ0tEiQ9sygY4hO/jRJd44hChdPca6xLb9RAjrktSpkpgTsqgBrcd1vhCLPjw3cHS1fgkH0UDM36K8enEvn398JfJQJApUpJrV7iq5kI/q08K042b9ZbGKMRdN/Uz/fSsN84z5zgJ8Cv4lngNd70EWi1DxpARWYvwxhyn/NrylJf9Z946lQTk58R/GfI/uXcjTzmKygc+2NhYQfsHZB+ZCexX/OnrDUZLbutXPRiwNK1Ko9cQ4EbXn0T6mf1317R6qrsIazSeZEJFZ86QhGmULb6Zdw4xWX1wwaQSbswDTRA5AF1bctD0v8H4WLBDuSbsekCK8Gpo4x9KTpSgGlZuf8W3q+Ad4ZrFLuRhBxftWMfnMq5dBoab/+FxklFFL/kjOgnXT65HDDgkAyPmChJmLW2NRIN2NhED9ORlcI2csfF6PxBZQLSlejN3W7/H0ECbI/5wp4jCYtB++mq9xrMN0BPmcj7bLLXGpt+3v2H9N4fv3AYB5aEYjUtsmFEC6/9glAqr94FrteSwTNy0H6JIdlYAaO3G3s8OtCacQeNLXKHjv3qbd8vKrsfGn+z053knmI8BIdTm+mieCzujeptYVyWPOEdDRVPKV+cy1cNAsGiilLuaUTKB4vYsIvicx4CLKyuSikM2wf99/qC258QH1km/HNqdagK/AyRcfWg/w3rGw= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: merged the two into cifs-2.6.git for-next pending additional testing and review (added Cc: stable as well) On Thu, Nov 30, 2023 at 6:22=E2=80=AFPM David Howells = wrote: > > Fix a number of issues in the cifs filesystem implementation of the FICLO= NE > ioctl in cifs_remap_file_range(). This is analogous to the previously > fixed bug in cifs_file_copychunk_range() and can share the helper > functions. > > Firstly, the invalidation of the destination range is handled incorrectly= : > We shouldn't just invalidate the whole file as dirty data in the file may > get lost and we can't just call truncate_inode_pages_range() to invalidat= e > the destination range as that will erase parts of a partial folio at each > end whilst invalidating and discarding all the folios in the middle. 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 each end > 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 clone reque= st > is issued, this will fail on the server with STATUS_INVALID_VIEW_SIZE > (which gets translated to -EIO locally) if the clone source extends past > the server's EOF. > > Fix this by: > > (0) Flush the source region (already done). The flush does nothing and > 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 at > least at this point. If we can't do this, for instance if the serve= r > doesn't support it, just flush the entire source file. > > (2) Find the folio (if present) at each end of the range, flushing it an= d > 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 b= e > reloaded. > > (4) Then perform the extent duplication. > > Thirdly, set i_size after doing the duplicate_extents operation as this > value may be used by various things internally. stat() hides the issue > because setting ->time to 0 causes cifs_getatr() to revalidate the > attributes. > > These were causing the cifs/001 xfstest to fail. > > Fixes: 04b38d601239 ("vfs: pull btrfs clone API to vfs layer") > Signed-off-by: David Howells > cc: Steve French > cc: Christoph Hellwig > 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 | 68 +++++++++++++++++++++++++++++++++++------- > 1 file changed, 57 insertions(+), 11 deletions(-) > > diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c > index 8097a9b3e98c..c5fc0a35bb19 100644 > --- a/fs/smb/client/cifsfs.c > +++ b/fs/smb/client/cifsfs.c > @@ -1268,9 +1268,12 @@ static loff_t cifs_remap_file_range(struct file *s= rc_file, loff_t off, > { > struct inode *src_inode =3D file_inode(src_file); > struct inode *target_inode =3D file_inode(dst_file); > + struct cifsInodeInfo *src_cifsi =3D CIFS_I(src_inode); > + struct cifsInodeInfo *target_cifsi =3D CIFS_I(target_inode); > struct cifsFileInfo *smb_file_src =3D src_file->private_data; > - struct cifsFileInfo *smb_file_target; > - struct cifs_tcon *target_tcon; > + struct cifsFileInfo *smb_file_target =3D dst_file->private_data; > + struct cifs_tcon *target_tcon, *src_tcon; > + unsigned long long destend, fstart, fend, new_size; > unsigned int xid; > int rc; > > @@ -1281,13 +1284,13 @@ static loff_t cifs_remap_file_range(struct file *= src_file, loff_t off, > > xid =3D get_xid(); > > - if (!src_file->private_data || !dst_file->private_data) { > + if (!smb_file_src || !smb_file_target) { > rc =3D -EBADF; > cifs_dbg(VFS, "missing cifsFileInfo on copy range src fil= e\n"); > goto out; > } > > - smb_file_target =3D dst_file->private_data; > + src_tcon =3D tlink_tcon(smb_file_src->tlink); > target_tcon =3D tlink_tcon(smb_file_target->tlink); > > /* > @@ -1300,20 +1303,63 @@ static loff_t cifs_remap_file_range(struct file *= src_file, loff_t off, > if (len =3D=3D 0) > len =3D src_inode->i_size - off; > > - cifs_dbg(FYI, "about to flush pages\n"); > - /* should we flush first and last page first */ > - truncate_inode_pages_range(&target_inode->i_data, destoff, > - PAGE_ALIGN(destoff + len)-1); > + cifs_dbg(FYI, "clone range\n"); > + > + /* Flush the source buffer */ > + rc =3D filemap_write_and_wait_range(src_inode->i_mapping, off, > + off + len - 1); > + if (rc) > + goto unlock; > + > + /* The server-side copy will fail if the source crosses the EOF m= arker. > + * Advance the EOF marker after the flush above to the end of the= range > + * if it's short of that. > + */ > + if (src_cifsi->netfs.remote_i_size < off + len) { > + rc =3D cifs_precopy_set_eof(src_inode, src_cifsi, src_tco= n, xid, off + len); > + if (rc < 0) > + goto unlock; > + } > + > + new_size =3D destoff + len; > + destend =3D destoff + len - 1; > > - if (target_tcon->ses->server->ops->duplicate_extents) > + /* Flush the folios at either end of the destination range to pre= vent > + * accidental loss of dirty data outside of the range. > + */ > + fstart =3D destoff; > + fend =3D destend; > + > + rc =3D cifs_flush_folio(target_inode, destoff, &fstart, &fend, tr= ue); > + if (rc) > + goto unlock; > + rc =3D cifs_flush_folio(target_inode, destend, &fstart, &fend, fa= lse); > + if (rc) > + goto unlock; > + > + /* Discard all the folios that overlap the destination region. */ > + cifs_dbg(FYI, "about to discard pages %llx-%llx\n", fstart, fend)= ; > + truncate_inode_pages_range(&target_inode->i_data, fstart, fend); > + > + fscache_invalidate(cifs_inode_cookie(target_inode), NULL, > + i_size_read(target_inode), 0); > + > + rc =3D -EOPNOTSUPP; > + if (target_tcon->ses->server->ops->duplicate_extents) { > rc =3D target_tcon->ses->server->ops->duplicate_extents(x= id, > smb_file_src, smb_file_target, off, len, destoff)= ; > - else > - rc =3D -EOPNOTSUPP; > + if (rc =3D=3D 0 && new_size > i_size_read(target_inode)) = { > + truncate_setsize(target_inode, new_size); > + netfs_resize_file(&target_cifsi->netfs, new_size)= ; > + fscache_resize_cookie(cifs_inode_cookie(target_in= ode), > + new_size); > + } > + } > > /* force revalidate of size and timestamps of target file now > that target is updated on the server */ > CIFS_I(target_inode)->time =3D 0; > +unlock: > /* although unlocking in the reverse order from locking is not > strictly necessary here it is a little cleaner to be consisten= t */ > unlock_two_nondirectories(src_inode, target_inode); > > --=20 Thanks, Steve