Re: [PATCH 01/39] vfs: dedpue: return loff_t

Re: [PATCH 01/39] vfs: dedpue: return loff_t

[Christoph Hellwig]

On Tue, May 29, 2018 at 04:43:01PM +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).

Can we just make it return 0 vs errno?  The only time we return
a different length than the one passed in is due to the btrfs cap.

Given that this API started out on btrfs we should just do the cap
everywhere to not confuse userspace.

Re: [PATCH 01/39] vfs: dedpue: return loff_t

[Miklos Szeredi]

On Mon, Jun 4, 2018 at 10:43 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, May 29, 2018 at 04:43:01PM +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).
>
> Can we just make it return 0 vs errno?  The only time we return
> a different length than the one passed in is due to the btrfs cap.
>
> Given that this API started out on btrfs we should just do the cap
> everywhere to not confuse userspace.

And that's a completely arbitrary cap; sure btrfs started out with
that, but there's no fundamental reason for that becoming the global
limit.  Xfs now added a different, larger limit, so based on what
reason should that limit be reduced?

I don't care either way, but at this stage I'm not going to change
this patch, unless there's a very good reason to do so, because if I
do someone will come and suggest another improvement, ad-infinitum...

Thanks,
Miklos

Re: [PATCH 01/39] vfs: dedpue: return loff_t

[Darrick J. Wong]

On Tue, Jun 05, 2018 at 10:33:22AM +0200, Miklos Szeredi wrote:
> On Mon, Jun 4, 2018 at 10:43 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Tue, May 29, 2018 at 04:43:01PM +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).
> >
> > Can we just make it return 0 vs errno?  The only time we return
> > a different length than the one passed in is due to the btrfs cap.
> >
> > Given that this API started out on btrfs we should just do the cap
> > everywhere to not confuse userspace.
> 
> And that's a completely arbitrary cap; sure btrfs started out with
> that, but there's no fundamental reason for that becoming the global
> limit.  Xfs now added a different, larger limit, so based on what
> reason should that limit be reduced?
> 
> I don't care either way, but at this stage I'm not going to change
> this patch, unless there's a very good reason to do so, because if I
> do someone will come and suggest another improvement, ad-infinitum...

I think we should hoist the MAX_RW_COUNT/2 limit to the VFS helpers
since afaict we generally cap max IO per call at MAX_RW_COUNT.  (I
probably should've capped ocfs2 back when I did xfs, but forgot).  If
btrfs wants to keep their lower (16M) limit then they're free to do so;
the interface documentation allows for this.  One of the btrfs
developers seems to be working on a patch series to raise the limit[1]
anyway.

--D

[1] https://www.spinics.net/lists/linux-btrfs/msg78392.html

> 
> Thanks,
> Miklos

Re: [PATCH 01/39] vfs: dedpue: return loff_t

[Miklos Szeredi]

On Wed, Jun 6, 2018 at 5:09 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> On Tue, Jun 05, 2018 at 10:33:22AM +0200, Miklos Szeredi wrote:
>> On Mon, Jun 4, 2018 at 10:43 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> > On Tue, May 29, 2018 at 04:43:01PM +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).
>> >
>> > Can we just make it return 0 vs errno?  The only time we return
>> > a different length than the one passed in is due to the btrfs cap.
>> >
>> > Given that this API started out on btrfs we should just do the cap
>> > everywhere to not confuse userspace.
>>
>> And that's a completely arbitrary cap; sure btrfs started out with
>> that, but there's no fundamental reason for that becoming the global
>> limit.  Xfs now added a different, larger limit, so based on what
>> reason should that limit be reduced?
>>
>> I don't care either way, but at this stage I'm not going to change
>> this patch, unless there's a very good reason to do so, because if I
>> do someone will come and suggest another improvement, ad-infinitum...
>
> I think we should hoist the MAX_RW_COUNT/2 limit to the VFS helpers
> since afaict we generally cap max IO per call at MAX_RW_COUNT.

I don't quite get it.   That MAX_RW_COUNT is to protect against
overflows in signed int.

Here we have a 64bit interface, so that's irrelevant, we can invent
any cap we want.  Lets choose our favorite bike shed size.  Mine is
1G.  But if that turns out too limiting it can be raised arbitrarily
later.

>  (I
> probably should've capped ocfs2 back when I did xfs, but forgot).  If
> btrfs wants to keep their lower (16M) limit then they're free to do so;
> the interface documentation allows for this.  One of the btrfs
> developers seems to be working on a patch series to raise the limit[1]
> anyway.

Yep, that got upstreamed now.  Which is good, we can just return zero
or error from ->dedupe_file_range() and be done with that.

Thanks,
Miklos