On Mon, May 7, 2018 at 2:17 PM, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > On Mon, May 07, 2018 at 01:32:09PM +0200, Miklos Szeredi wrote: >> On Mon, May 7, 2018 at 1:11 PM, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: >> > On Mon, May 07, 2018 at 10:21:06AM +0200, Miklos Szeredi wrote: >> >> f_op->dedupe_file_range() gets a u64 length to dedup and returns an ssize_t >> >> actual length deduped. This breaks badly on 32bit archs since the returned >> >> length will be truncated and possibly overflow into the sign bit (xfs and >> >> ocfs2 are affected, btrfs limits actual length to 16MiB). >> >> >> >> Returning s64 should be good, since clone_verify_area() makes sure that the >> >> supplied length doesn't overflow. >> > >> > Why s64 rather than loff_t? Particularly since the next patch turns >> > the paramters into loff_t. >> >> Next patch turns the offsets into loff_t and leaves "len" as u64. A >> size is definitely not an offset, I'd consider changing the type of >> "len" to loff_t a misuse. > > Usually a size is the size of something in memory. The length of > something on storage is definitely an loff_t. Look at fallocate() > for an example. You could also argue that lseek where whence is set to > anything other than SEEK_SET is also being used as a length rather than > an absolute offset. We also already use 'loff_t len' as an argument to > vfs_dedupe_file_range_compare(). Fair enough. Will fix. Thanks, Miklos
