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 2570AC07CA9 for ; Thu, 30 Nov 2023 17:27:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4E1806B03FF; Thu, 30 Nov 2023 12:27:25 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 491A86B0416; Thu, 30 Nov 2023 12:27:25 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 332DD6B0417; Thu, 30 Nov 2023 12:27:25 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 23DF96B03FF for ; Thu, 30 Nov 2023 12:27:25 -0500 (EST) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id D8CAFA0325 for ; Thu, 30 Nov 2023 17:27:24 +0000 (UTC) X-FDA: 81515302008.10.8C8B272 Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) by imf04.hostedemail.com (Postfix) with ESMTP id 7DDC74000E for ; Thu, 30 Nov 2023 17:27:21 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=NjE7nGaZ; spf=pass (imf04.hostedemail.com: domain of smfrench@gmail.com designates 209.85.218.48 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=1701365241; 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=SicxnPAtP6HNpzxxsxCGJVDSWVb1qgmi8jAyrMaP4xE=; b=wMSIWVdI2gj8QMQQz0BJ2xrQVXS5iallJT9H1v5RtvjS5LBWkkTt9468f0h3Uc8zrTyNw7 g7y2SPkFGWnk+wOnyLJ/Wu7tAVzaIpFhx/70AeXJsskF+4NEn2sRXfxbRxPqAlLsZMC+OM 5TDnIaLrhtozKeorR8iMEAOapd0D2uQ= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701365241; a=rsa-sha256; cv=none; b=kU98qAtIo2N+xFgjrghVY8cbbnXuQ7wKSBh7rNflnC5Ak7CzlEADLTudqmYb5Z073ZK3o3 NE9bNGgWinXRp0w2Ydd65qRXk1FwP2fTz4RUtB64N8kOJGK22aiZc0AUCRR8oUYK4K5ysi x3O/7loMOw+Np7TWt2MFT7QFIoUm2lw= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=NjE7nGaZ; spf=pass (imf04.hostedemail.com: domain of smfrench@gmail.com designates 209.85.218.48 as permitted sender) smtp.mailfrom=smfrench@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ej1-f48.google.com with SMTP id a640c23a62f3a-a00191363c1so169627366b.0 for ; Thu, 30 Nov 2023 09:27:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701365240; x=1701970040; 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=SicxnPAtP6HNpzxxsxCGJVDSWVb1qgmi8jAyrMaP4xE=; b=NjE7nGaZ+r8sxGWTGpzvKZCQAvwRK49yP+I157I3wlsrJpXoC74vz8Xj4oPbu/Ngxs WYOjjMrp1nZ+dcL+sA3LamWI4fQI+2uPSEYJBnTkD9RAXTitoiWSbnEdzlSKkqbg7FtQ 1yge0xQbbJLXNBf5dyAL4UtFve9jEKlkMgSI0MOyK7dIvp+HrDaYVctJ1dKBczIMetPe d6bfhmZHTlYmp8aodaUGakG1pzCh1SWueFRz9vCvb765l+pkf+OWlXJmcL4bOqdShBBV xZYWqtosOwCLnenmpbAP36reMdat6gMue9RfbkBE662l9OZYmZoR6+uM4LTi1mHq3TdQ T67w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701365240; x=1701970040; 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=SicxnPAtP6HNpzxxsxCGJVDSWVb1qgmi8jAyrMaP4xE=; b=H6K/iJOF62P0ZoUTsIwch75GOWRBmq3Po6HQsFCLPrlJtQ2RMQ2rygYBi4E2rSXp9u yZHg5JZSz9yZEnza9rSVzhS8079mSVqXIxWYPOJn+KNCAh1Imdsx3iUMSl3iZ6YLC9Is KIqYe7UDZ0tfVzS9wLcfr1x1kcL5leJxJ7CCugosVlhRNx1yZ4ekArBC5C3hQtvfvg2f qQAcu2KaACjjyMYA7sl5eEZaLfw/bNiGpMVUSJTEPIncX1UnEIi/TDo99QHx/l/UPeG8 W9PR14Qitu97uHPgv+wBMwFLqhBTfO23d7qchBAAZ/Ay3YCNOBlrafmZ9qjOFfedqFFD h0GQ== X-Gm-Message-State: AOJu0Yx1kQNS/5CFgy10qk5/A5V5GPui4fLdEooRFgaJfWzHog6gUjac Sbafnh1NK7bSiHit25GEsBBrg4xvmO8/cL+l6q4ZmjdFNGDbgA== X-Google-Smtp-Source: AGHT+IF9AHPVeHOj80hoFrSbN5SdYfcMUZU7DK/ecVQVaMFPsYhqgizG5kwDo0XFHsh2y8ifJJzaYiTEJNS+paYjrpc= X-Received: by 2002:ac2:5e75:0:b0:500:b2f6:592 with SMTP id a21-20020ac25e75000000b00500b2f60592mr8755lfr.50.1701364154839; Thu, 30 Nov 2023 09:09:14 -0800 (PST) MIME-Version: 1.0 References: <20231129165619.2339490-1-dhowells@redhat.com> <20231129165619.2339490-4-dhowells@redhat.com> In-Reply-To: From: Steve French Date: Thu, 30 Nov 2023 11:08:43 -0600 Message-ID: Subject: Re: [PATCH 3/3] cifs: Fix flushing, invalidation and file size with copy_file_range() To: David Howells Cc: 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: 7DDC74000E X-Rspam-User: X-Stat-Signature: ucuidbbkp56m3uhqg85wfpt7587d1ood X-Rspamd-Server: rspam03 X-HE-Tag: 1701365241-83003 X-HE-Meta: U2FsdGVkX1+FfJchz6C5Y7geFPVngfbCe+yTh7fIQrspRezfWybCO/H24T2c2yfnmcX4xWIVMeZz/0qHYhPZq6JJx0y8mBPS7tm+XBFuZiVRU9tWpLws6T+melFFt6gc7D4s4csPs60LL+9TezemY4ufc6PQL+eE18yIe6ITwm8LQ5wcwzv3Akki1bIv7xg1JxoN0ACMwjem1TjpMVCCZoOPUhGeL6xJa4WcvmABukKecjVl4ojl+gl+wd6+73kMgQDKB08D+MukKEdxKTHKtQIlCqmb8g71yWNQpN6IMosL6VfaJ3c6WTOxh29zNs5h3XBgV7xahhd0qyuWC4z482dJXElTBfsSUujSoNm3Uwzl7xvHewKTEW0n5dmdpBUAmqJoi9bcI5ScIUZlq1g1eT4j9yCSEYDxlmZ92o4DBZQmJ0N+LwJ4I1ktciJ/6uyEm+ZL8YO8mEAeaXgzseHFCUa1pnWM9gjugglZgyJZqv+/IXm2gTqyEgzVRuavGdcloFq0M5RrJTodFfeFTd26obg02Nfl8/qwjQeMczZgcU5iITISkaiN4GE4PtiVaffyhenEcf+74cMprlFcP6zXw+j2t2+Uimnv2FHLY+YfLED07+Qm2+X27sFqave8UBklHLJFHBm4UqvfSKW7oEO9cC4RqDfJS+4viOtJ1ZdQUnloZNUXzT+9SYFXvO93psF95egfvmZcnPwGLMUpG2eyqfx7jBQvmKn4H4Tpzry3BDja0o/6w5bY1M2/zDrzWM8XLVORc0050AZVxu52cMtlmeP6e3nT7j2xxN7oNgiZAECvRy9kGa3kHVDW3VlXt319ElyfzVgvNEJyYb726744Y0zo6FWaOXAAJrrjxZxQQSAD86bais99WuZOnkOYHeB6p6cLhmv1FE9Iwwv3GCzVSLKCTAmEN8thJEWaQLetamDcw2JnVXN6smhOxpI6FAfxH0uAdxIwlAwPWe95vdI OeMiGAKU hUkSkT4jd5LR/JeElwRLfQqjulMqlUeWfMCczOumE5QpRGvB2VYnwMjqMZpdxHX1Z/Cy/ul5y8aCkTM9Jkx9cqCehiwgqD1xd+0oJ74WjA4CUl16VH+xZEvl8FwWk7/5UlTsxowujrtSglrR4NSqZYp5TEUqKLIJtRAi1Baiq6asiH4F257j8mUCWipU0TM2gT0d74cR7/8E7gMpUt8MQoInEURP1jW+EzDpn/sjKtZLQ3lFuHd6D1JXH4YO1Vrwd2oe9KP6OF1rUiNJS23/RxEp0UBldh3yWN4DR37hyXUkJzpRILzSeNSTKZGMqHJnTsjvwrTPBZIqTyQnrOaBFZEgvkBbN9p3obe07vAUYd9mFE/mxByQxbkNRzA+GTiVMcOBQeSQUkJfFlFkLplVKxVCqHCcZs1uzU2kxZzDgm44gHeTn//3p1mINNaSR1pGOTWfHybTwreOyfOuGaT4s5O+JCAFuW3cGkUq70rdcNUQ7hG3JMVdW/73eDXop6mw289ZJt3SOlUViOwmIUaD2ZhgfNWfcMP3YAzYfL8r0cn1BBHDOR5qI6srSStlAlDdq0+qWdaeIaln/pz1KbK6SlgsMO0qABa0vKv/JzoXceTOqa9YSFbKtbpQqnLG8CTlhGWfX63RjMuEHe7XAmBh+shJfSxWkGnRP6vGzn7+GZdlWvvkxD8GByMNhgRGLJCPVE1ywJahdqMY5ukKLl0gaXIdjMIqlKWEdHz3i2k2h3spRP5UteQLaEJrRFdjDLgSNnIvZO8W2ayweeI8tGEcePZfzfNyVPk4cH6cX6wa6ambWQrxPPpzFkvDXww== 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: There is a minor problem with the patch in the change to cifs_file_copychunk_range() in cifsfs.c. With this change it can attempt to set the file size using a file handle without write permission (in this path it is common for the source file to be opened for read when doing a copy). Fortunately I can't reproduce that in any of my tests (because the file size is up to date and data from source file was already flushed) but safer to fix it. /* The server-side copy will fail if the source crosses the EOF mar= ker. * Advance the EOF marker after the flush above to the end of the r= ange * 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); This should be calling the path based equivalent to set the file size so it can find a writeable file. On Wed, Nov 29, 2023 at 3:37=E2=80=AFPM Steve French w= rote: > > 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 incorrect= ly: > > 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 invalid= ate > > 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 requ= est > > 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 v= alue > > 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(-) > > > > 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 = *src_file, loff_t off, > > return rc < 0 ? rc : len; > > } > > > > +/* > > + * Flush out either the folio that overlaps the beginning of a range i= n which > > + * pos resides (if _fstart is given) or the folio that overlaps the en= d of a > > + * range (if _fstart is NULL) unless that folio is entirely within the= range > > + * we're going to invalidate. > > + */ > > +static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_= fstart, 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, fen= d); > > +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 = xid, > > { > > 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 = xid, > > 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= marker. > > + * Advance the EOF marker after the flush above to the end of t= he 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 p= revent > > + * 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, &f= end); > > + 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(x= id, > > smb_file_src, smb_file_target, off, len, destof= f); > > + if (rc > 0 && destoff + rc > i_size_read(target_inode)) > > + truncate_setsize(target_inode, destoff + rc); > > + } > > > > file_accessed(src_file); > > > > > > > > > -- > Thanks, > > Steve --=20 Thanks, Steve