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 marker.
* 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 = 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 PM Steve French <smfrench@xxxxxxxxx> wrote:
>
> 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 AM David Howells <dhowells@xxxxxxxxxx> 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 invalidate
> > 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 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 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 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 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 <dhowells@xxxxxxxxxx>
> > cc: Steve French <sfrench@xxxxxxxxx>
> > cc: Paulo Alcantara <pc@xxxxxxxxxxxxx>
> > cc: Shyam Prasad N <nspmangalore@xxxxxxxxx>
> > cc: Rohith Surabattula <rohiths.msft@xxxxxxxxx>
> > cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > cc: Jeff Layton <jlayton@xxxxxxxxxx>
> > cc: linux-cifs@xxxxxxxxxxxxxxx
> > cc: linux-mm@xxxxxxxxx
> > ---
> > 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 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 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 = pos / PAGE_SIZE;
> > + size_t size;
> > + int rc = 0;
> > +
> > + folio = filemap_get_folio(inode->i_mapping, index);
> > + if (IS_ERR(folio)) {
> > + if (_fstart)
> > + *_fstart = pos;
> > + *_fend = pos;
> > + return 0;
> > + }
> > +
> > + size = folio_size(folio);
> > + fpos = folio_pos(folio);
> > + fend = fpos + size - 1;
> > + if (_fstart)
> > + *_fstart = fpos;
> > + *_fend = fend;
> > + if (_fstart && pos == fpos)
> > + goto out;
> > + if (!_fstart && pos == fend)
> > + goto out;
> > +
> > + rc = 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 xid,
> > {
> > struct inode *src_inode = file_inode(src_file);
> > struct inode *target_inode = file_inode(dst_file);
> > + struct cifsInodeInfo *src_cifsi = 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 the range
> > + * if it's short of that.
> > + */
> > + if (src_cifsi->server_eof < off + len) {
> > + rc = 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 = destoff + len - 1;
> > +
> > + /* Flush the folios at either end of the destination range to prevent
> > + * accidental loss of dirty data outside of the range.
> > + */
> > + fstart = destoff;
> > +
> > + rc = cifs_flush_folio(target_inode, destoff, &fstart, &fend);
> > + if (rc)
> > + goto unlock;
> > + if (destend > fend) {
> > + rc = cifs_flush_folio(target_inode, destend, NULL, &fend);
> > + if (rc)
> > + goto unlock;
> > + }
> > +
> > + /* Discard all the folios that overlap the destination region. */
> > + truncate_inode_pages_range(&target_inode->i_data, fstart, fend);
> >
> > rc = file_modified(dst_file);
> > - if (!rc)
> > + if (!rc) {
> > rc = 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);
> >
> >
> >
>
>
> --
> Thanks,
>
> Steve
--
Thanks,
Steve
