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 A2E34C07CB1 for ; Wed, 29 Nov 2023 21:37:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3A9236B03E9; Wed, 29 Nov 2023 16:37:45 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3325D6B03EA; Wed, 29 Nov 2023 16:37:45 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1ABD26B03EB; Wed, 29 Nov 2023 16:37:45 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 04CAD6B03E9 for ; Wed, 29 Nov 2023 16:37:45 -0500 (EST) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id D49F0A0779 for ; Wed, 29 Nov 2023 21:37:44 +0000 (UTC) X-FDA: 81512304048.18.4FD7453 Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com [209.85.167.54]) by imf14.hostedemail.com (Postfix) with ESMTP id CE29D100014 for ; Wed, 29 Nov 2023 21:37:42 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Zkd1P7uq; spf=pass (imf14.hostedemail.com: domain of smfrench@gmail.com designates 209.85.167.54 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=1701293863; 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=h6iDu2i/opAV7vxgDJpFdxC85eK3u1f4pMUMuRKAlTE=; b=TLMHVjPiEC9m1GjGAGzIuplQop+dfcpELOPDExa7X/DIl5DK41DhFdZYc47SA+rUlfUfeo 1rhjqY4GFRzieDBiwzOR6TU3SyRWXp/BkEpMtKoMW4cxcrc0xYt2MacM5yqDzeS6630qKq OhmEyRgFGNQPlPcUPSED0TOAaIYi5R0= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701293863; a=rsa-sha256; cv=none; b=sPmBdEj8Cee10J6QvEk/oLgPzB3JQGZX+CFFKwVgcED/B1JHKTY+CtDKCaNCdkTxm9GWRb UOUDei2K7Gk49WqZLzTUBCjrcWHCjBsQssQxZklZcxe0iYiSqjh3wELGoSV6IaOOdc17Qm j/11fq9EzPZg7+jjsyNZAf4H2lHdZWc= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Zkd1P7uq; spf=pass (imf14.hostedemail.com: domain of smfrench@gmail.com designates 209.85.167.54 as permitted sender) smtp.mailfrom=smfrench@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-lf1-f54.google.com with SMTP id 2adb3069b0e04-50943ccbbaeso412305e87.2 for ; Wed, 29 Nov 2023 13:37:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701293861; x=1701898661; 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=h6iDu2i/opAV7vxgDJpFdxC85eK3u1f4pMUMuRKAlTE=; b=Zkd1P7uq+VAMJ8B+qrvT9U8Jd//xyGRaJpvWUtfs8qy3juvUFnubY1Kg1IL9gttcvQ VBiXA9VWt/S5tb5VFNclgD/iPy1Mb9R3dtq/8l0uXHA2NfSHVUWVmDgtpFakYGSI9WO4 De1hEoEqjXkFsJrGZlDCQEhRGW+IbptqL2FnDikNxwLtPwk4+bdtLQl4cUlROBQRhXy/ 7Wy27fKyNDl6GnAxXm2jHqsMhf5H7TqnlomAo1uMyHlvtbrZlGQwYqmJGWL33mdJP386 DEEKZBnA2jXikKSm+SayBlg18dkCTwWi/UnBJrDYyvHSDET9P6nnctpSzOba2G6yFRUt urWg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701293861; x=1701898661; 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=h6iDu2i/opAV7vxgDJpFdxC85eK3u1f4pMUMuRKAlTE=; b=TDIeZdJ5Ootodg4Jyg5zQQ5jU8mT9+KTCLFB+w9O47J8A1JJ4ZGEKN5IrZBJWOvIBb bsP2HgAYYBXdd6hrcVfdccopwUThbGxplkzj49SCVmjPnmjT6GxKE205js5KQ2qf/Di8 u8vE8krmqFtOJDe2NZTLZc7gkZxUgk3R6ll867D/7esFujrkmIBzyjrLx301zXUkUPxm 0k7nmitjQM03a90NrenPWiTgE8HXSKNVIyLz29jrSCu7/eaH7Jlu0x2ZN7bkgDSYnnp8 JzdnIpsipbGhJpI5wjFeor4346c3ik8LtQjkTphhgHaH57xCs9k0Nv3XYSV949jiI/Oa Z65w== X-Gm-Message-State: AOJu0YwcyGtvX9yzSLrAwR6p2ior4+Ffy1zkKyvy+oEKqcf0BCcmjn0i C+pDNV7Dz9U0H8HyHw7q7FDdhYCh6ZUaHb21/Hc= X-Google-Smtp-Source: AGHT+IG9gufZvE3jDqZ8qWDTq84pWk1Wg5aunbT+pxf5lF2ZvF617lVdwCFbfNpFc2UmAlV+h5jUfInWDoEdy/xggR4= X-Received: by 2002:a05:6512:3e24:b0:50b:c9b8:36b with SMTP id i36-20020a0565123e2400b0050bc9b8036bmr730049lfv.69.1701293860737; Wed, 29 Nov 2023 13:37:40 -0800 (PST) MIME-Version: 1.0 References: <20231129165619.2339490-1-dhowells@redhat.com> <20231129165619.2339490-4-dhowells@redhat.com> In-Reply-To: <20231129165619.2339490-4-dhowells@redhat.com> From: Steve French Date: Wed, 29 Nov 2023 15:37:29 -0600 Message-ID: Subject: Re: [PATCH 3/3] cifs: Fix flushing, invalidation and file size with copy_file_range() 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: CE29D100014 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: 9medh143p8n6j9sre6am156porw5q7yh X-HE-Tag: 1701293862-55356 X-HE-Meta: U2FsdGVkX1/sE1Uj9PGIEkigQxxmny1vmtkINsQEfWCnuKXXZM+d/2XG2L0dfWaHsfg9KZCIa4sxMefjVGgcMqj0Qoqc/ZXeBct9c0Om/tVYMaFI/bbe+NfZJHwP3aarBMqvRGGbQxbzCuyE5P73YS54Fj0blmJmiD2zAWYZLyR6Opj/bgsCh7U1hX1LB/Q7iWGC/IDFvzNS/5S7GNazOJzqjtQNT1D9Ap26hPNdFp9jDZYxCHxYAlMhW5EIBbajjvASfZkyNoJ+U3NMdYKIonydfAZP/rHu1xe5PTldmo8eFKrJRkFtyUy9qq+sHzBuwT1J9+eA/Q+PodSURbvQXLbiiGA+BepKrA/lpXwXNWNftXa6FDifSKKkYZuWXAa44GPaCqyLbaMSmNX2kgb0jMytyrufHsL1rA4K3tke89k0Zms0o1Pwn8MLCttJGUPoWmEWEjuIwyJDiyU58BsWvRae6HoBIJa6ifGhulAYVdPZypLScIXL6U3wci4rytCWGbgyR3BiFORLJxqK/qTHqCLKfsdbc3F8KgDxJhcWv2mF2Tu+z/vgBGg+J9jRAWBK3nvAPngT7Q/C84A7/m9wJuohY+nK20y4tYht8V4tTjH2L2gnUW83464J74mf671uIdsjUmSo0XILwLEbsGbf8p2JhkSmf21VxG42elvO22BWwNgp0guJXoaD7gAIgfi0FRjvRCZpSXiYwHrf/y8xaayD1HnQ+iWoXESMpRf4x/fsWEGHCTSAKUkSqATG3DTS7iUTxPKuaPN2ja2koO9a6IXGvOzzRTAOuxWXj5w7J/+7NZdXors//2qzZmHCAmXjLuWkSCleiyXQzcw0XrRtxCP07YJB2d/J8QCpmfcn/rMXWtUAJsgCysvTaTUfR2GS40HIC7+gZnfEW8eIZAWt9it0yiIj1OXDBB5KgnXYi5gV031xhZMPdUI0RYcwGGCtHWvVXv1kh+uJo+aW8yc P2BTwgWB sgaIfmP0lcndiIlP3gBV1pgYMHYFIW/GsRsnFJXjaZ2kWBsrkD6/HObHqIcCiNG24M2yyD0A/W2aGCZjMYHoE868q38poJvfGRGuK1D9izViUcvhkmiNqTWVZ/8egBDk0kQptGuQNujwf6Pm2sLR9E5sa7KiBJRs9L/uZB2Jgjaqu/d/kj/+3f6ZtyzzFHWp47qZBcWYb7rAx9hG1mwPq2t3d5rJdJJLJs1YxS+EJc6VmGfAN/V7da01mWjLVYcQ6J6N1bcN1iAST0fFuGNz/8UXwDhgnOF8FV70AWdFqOLDM8EIVl0SN5xuqL7WG3fng0D5pw6KUjRLDrtVrIag32AMPIp+5mRVnFh7Mfx/A+OFVYuBpqNf9KTSWqMAzbtKeRFgUuvXfa0oZc+f+VRXFzijJqzC1Jmv1olShRjN2wJ86TwntqDsDo+J0b0+8exYxapmysMp5MEyTf6c6+/FpOQgtUk461Ozs2Jl8UbyaX6HdONzNcVw6x6S5NPTuILIRO6E4+hSciR5YWxxAWgWIK6D5FGziGMnJpheZLvkWHLg9LDnrDmekuvng+m8dzWkZOMckPMlJ1GWgCK7KX76KLU4yP/H8W1qlleg5uopjMe3X9WZbvVT+Dbe5huWTbE+LBFyJRwBs03QIwYxZNhc91xPXb9gEZ+fdyxdkhUrAX4RJZhi1gsMUCgWeovMTaKW6szmbBuNTtYpMfdR36Nt11Ga+ht6wzRM/0m6wil3RAEb/1UU8O1nYrKyzUcQcGx+lZtWjFdvEqV7xTDyc9IZBrXwB0DcpWg0bzQHrLFt0ZPsKTnUsx/gqh/iDAA== 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: Fixed a minor whitespace issue, and tentatively added to cifs-2.6.git for-next (all three) pending additional testing On Wed, Nov 29, 2023 at 10:56=E2=80=AFAM David Howells wrote: > > 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 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 copy reques= t > 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 t= he > 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. > > [!] Rather than moving the EOF, it might be better to split the copy > 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 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 copy. > > Thirdly, set i_size after doing the copychunk_range operation as this val= ue > 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 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(-) > > diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c > index ea3a7a668b45..6db88422f314 100644 > --- a/fs/smb/client/cifsfs.c > +++ b/fs/smb/client/cifsfs.c > @@ -1256,6 +1256,45 @@ static loff_t cifs_remap_file_range(struct file *s= rc_file, loff_t off, > return rc < 0 ? rc : len; > } > > +/* > + * Flush out either the folio that overlaps the beginning of a range in = which > + * pos resides (if _fstart is given) or the folio that overlaps the end = of a > + * range (if _fstart is NULL) unless that folio is entirely within the r= ange > + * we're going to invalidate. > + */ > +static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fs= tart, loff_t *_fend) > +{ > + struct folio *folio; > + unsigned long long fpos, fend; > + pgoff_t index =3D pos / PAGE_SIZE; > + size_t size; > + int rc =3D 0; > + > + folio =3D filemap_get_folio(inode->i_mapping, index); > + if (IS_ERR(folio)) { > + if (_fstart) > + *_fstart =3D pos; > + *_fend =3D pos; > + return 0; > + } > + > + size =3D folio_size(folio); > + fpos =3D folio_pos(folio); > + fend =3D fpos + size - 1; > + if (_fstart) > + *_fstart =3D fpos; > + *_fend =3D fend; > + if (_fstart && pos =3D=3D fpos) > + goto out; > + if (!_fstart && pos =3D=3D fend) > + goto out; > + > + rc =3D filemap_write_and_wait_range(inode->i_mapping, fpos, fend)= ; > +out: > + folio_put(folio); > + return rc; > +} > + > ssize_t cifs_file_copychunk_range(unsigned int xid, > struct file *src_file, loff_t off, > struct file *dst_file, loff_t destoff, > @@ -1263,10 +1302,12 @@ ssize_t cifs_file_copychunk_range(unsigned int xi= d, > { > 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 cifsFileInfo *smb_file_src; > struct cifsFileInfo *smb_file_target; > struct cifs_tcon *src_tcon; > struct cifs_tcon *target_tcon; > + unsigned long long destend, fstart, fend; > ssize_t rc; > > cifs_dbg(FYI, "copychunk range\n"); > @@ -1306,13 +1347,46 @@ ssize_t cifs_file_copychunk_range(unsigned int xi= d, > if (rc) > goto unlock; > > - /* should we flush first and last page first */ > - truncate_inode_pages(&target_inode->i_data, 0); > + /* 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->server_eof < off + len) { > + rc =3D src_tcon->ses->server->ops->set_file_size( > + xid, src_tcon, smb_file_src, off + len, false); > + if (rc < 0) > + goto unlock; > + > + fscache_resize_cookie(cifs_inode_cookie(src_inode), > + i_size_read(src_inode)); > + } > + > + destend =3D destoff + len - 1; > + > + /* 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; > + > + rc =3D cifs_flush_folio(target_inode, destoff, &fstart, &fend); > + if (rc) > + goto unlock; > + if (destend > fend) { > + rc =3D cifs_flush_folio(target_inode, destend, NULL, &fen= d); > + if (rc) > + goto unlock; > + } > + > + /* Discard all the folios that overlap the destination region. */ > + truncate_inode_pages_range(&target_inode->i_data, fstart, fend); > > rc =3D file_modified(dst_file); > - if (!rc) > + if (!rc) { > rc =3D target_tcon->ses->server->ops->copychunk_range(xid= , > smb_file_src, smb_file_target, off, len, destoff)= ; > + if (rc > 0 && destoff + rc > i_size_read(target_inode)) > + truncate_setsize(target_inode, destoff + rc); > + } > > file_accessed(src_file); > > > --=20 Thanks, Steve