The mapping->invalidate_lock, copy-offload and cifs

The mapping->invalidate_lock, copy-offload and cifs

[David Howells]

Hi Jann,

I'm looking at trying to fix cifs_file_copychunk_range().  Currently, it
invalidates the destination range, apart from a partial folio at either end
which will be flushed, and then tries the copy.  But if the copy fails or can
only be partially completed (eg. ENOSPC), we lose any data in the destination
region, so I think it needs to be flushed and invalidated rather than just
being invalidated.

Now, we have filemap_invalidate_inode() which I can use to flush back and
invalidate the folios under the invalidate_lock (thereby avoiding the need for
launder_folio).  However, that doesn't prevent mmap from reinstating the
destination folios with modifications whilst the copy is ongoing the moment
the invalidate_lock is dropped.

Question is: would it be reasonable to do the copy offload whilst holding the
invalidate_lock for the duration?

Thanks,
David

Re: The mapping->invalidate_lock, copy-offload and cifs

[Jan Kara]

Hi David!

On Tue 27-08-24 15:46:55, David Howells wrote:
> I'm looking at trying to fix cifs_file_copychunk_range().  Currently, it
> invalidates the destination range, apart from a partial folio at either end
> which will be flushed, and then tries the copy.  But if the copy fails or can
> only be partially completed (eg. ENOSPC), we lose any data in the destination
> region, so I think it needs to be flushed and invalidated rather than just
> being invalidated.
> 
> Now, we have filemap_invalidate_inode() which I can use to flush back and
> invalidate the folios under the invalidate_lock (thereby avoiding the need for
> launder_folio).  However, that doesn't prevent mmap from reinstating the
> destination folios with modifications whilst the copy is ongoing the moment
> the invalidate_lock is dropped.
> 
> Question is: would it be reasonable to do the copy offload whilst holding the
> invalidate_lock for the duration?

FWIW yes, I'd expect cifs_file_copychunk_range() to take invalidate_lock on
the target file to avoid possible races with page faults. We do it this
already for similar operations such as reflink or various fallocate
operations...

								Honza

> 
> Thanks,
> David
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR