[Devel] FIEMAP support

[Alexey Kuznetsov]

Hello!

On Fri, Jul 7, 2017 at 9:03 AM, Maxim Patlasov <mpatlasov at virtuozzo.com> wrote:
>> +    if (((linear_off + sizeof(struct fiemap_extent)) / PAGE_SIZE) ==
>> (linear_off / PAGE_SIZE)) {
>
> If "linear_off + sizeof(struct fiemap_extent)" is multiple of PAGE_SIZE, the
> condition is false, while obviously no split needed. I'd suggest to fix it
> like this:
>
>> +    if (((linear_off + sizeof(struct fiemap_extent) - 1) / PAGE_SIZE) ==
>> (linear_off / PAGE_SIZE)) {

Yup. Shit happens. Plain luck it works.


>> +        if (ofiemap.fm_mapped_extents > cur_max) {
>> +            err = -EINVAL;
>
> Can we use something more descriptive than EINVAL for such a rare and
> unlikely error? Are you OK about EIO?

OK. Actually, it is an impossible situation meaning a bug in user space fused.


>> +        if (last_p && (fe.fe_flags & FIEMAP_EXTENT_LAST))
>
> last_p cannot be NULL here. you assign it to '1' a few lines above without
> any checks.

Yes. It is NULL only if fi_extents_max==0, which is already eliminated.

>
>> +            *last_p = 1;
>> +        next_start = fe.fe_logical + fe.fe_length;
>> +        if (next_start >= *start_p + *len_p)
>> +            *len_p = 0;
>> +        else
>> +            *len_p -= *start_p + *len_p - next_start;
>
> Should it be simply "=" (without minus)?

Shit... Sorry. How did it work? I tested it on several thousands
of extents...

And the case when buggy fused returns an extent < *start_p also
should be handled, otherwise kernel could loop.

I feel you wasted more time to read this than I spent to write this. :-)


>> +int fuse_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>> +        __u64 start, __u64 len)
>> +{
>> +    struct fuse_conn *fc = get_fuse_conn(inode);
>> +    u32 cur_max;
>
> this cur_max is unused variable

Yeah, I moved it inside the loop...



>> +    if (!(fieinfo->fi_flags & FIEMAP_FLAG_SYNC)) {
>> +        printk(KERN_DEBUG "fuse fiemap w/o sync %s[%u]\n", current->comm,
>> current->pid);
>
> I'd suggest to ratelimit this printk because it's under user control to
> busy-loop over fiemap() w/o FLAG_SYNC.

Of course. Or even delete.