On 15.10.20 17:58, Kevin Wolf wrote: > Am 15.10.2020 um 17:18 hat Max Reitz geschrieben: >> On 15.10.20 11:46, Kevin Wolf wrote: >>> Am 22.09.2020 um 12:49 hat Max Reitz geschrieben: >>>> This makes the export actually useful instead of only producing errors >>>> whenever it is accessed. >>>> >>>> Signed-off-by: Max Reitz <[email protected]> > >>>> +/** >>>> + * Handle client reads from the exported image. >>>> + */ >>>> +static void fuse_read(fuse_req_t req, fuse_ino_t inode, >>>> + size_t size, off_t offset, struct fuse_file_info >>>> *fi) >>>> +{ >>>> + FuseExport *exp = fuse_req_userdata(req); >>>> + int64_t length; >>>> + void *buf; >>>> + int ret; >>>> + >>>> + /** >>>> + * Clients will expect short reads at EOF, so we have to limit >>>> + * offset+size to the image length. >>>> + */ >>>> + length = blk_getlength(exp->common.blk); >>>> + if (length < 0) { >>>> + fuse_reply_err(req, -length); >>>> + return; >>>> + } >>>> + >>>> + size = MIN(size, FUSE_MAX_BOUNCE_BYTES); >>> >>> "Read should send exactly the number of bytes requested except on EOF or >>> error, otherwise the rest of the data will be substituted with zeroes." >> >> :( >> >>> Do we corrupt huge reads with this, so that read() succeeds, but the >>> buffer is zeroed above 64M instead of containing the correct data? Maybe >>> we should return an error instead? >> >> Hm. It looks like there is a max_read option obeyed by the kernel >> driver, and it appears it’s set by implementing .init() and setting >> fuse_conn_info.max_read (also, “for the time being” it has to also set >> in the mount options passed to fuse_session_new()). >> >> I’m not sure whether that does anything, though. It appears that >> whenever I do a cached read, it caps out at 128k (which is what >> cuse_prep_data() in libfuse sets; though increasing that number there >> doesn’t change anything, so I think that’s just a coincidence), and with >> O_DIRECT, it caps out at 1M. >> >> But at least that would be grounds to return an error for >64M requests. >> (Limiting max_read to 64k does limit all read requests to 64k.) > > Yes, setting max_read and making larger requests an error seems like a > good solution. > >> Further investigation is needed. > > If you want :-)
Not really, but 128k per request is a bit sad.
[...]
>>>> +/**
>>>> + * Let clients flush the exported image.
>>>> + */
>>>> +static void fuse_flush(fuse_req_t req, fuse_ino_t inode,
>>>> + struct fuse_file_info *fi)
>>>> +{
>>>> + FuseExport *exp = fuse_req_userdata(req);
>>>> + int ret;
>>>> +
>>>> + ret = blk_flush(exp->common.blk);
>>>> + fuse_reply_err(req, ret < 0 ? -ret : 0);
>>>> +}
>>>
>>> This seems to be an implementation for .fsync rather than for .flush.
>>
>> Wouldn’t fsync entail a drain?
>
> Hm, the libfuse documentation doesn't say anything about draining. I
> suppose this is because if requests need to be drained, it will do so in
> the kernel. But I haven't checked the code.
Hmm, well, yeah... A sync doesn’t necessarily mean settling all
in-flight requests.
> So I expect that calling fsync() on the lower layer does everything that
> is needed. Which is bdrv_flush() in QEMU.
>
>> Or is it the opposite, flush should just drain and not invoke
>> blk_flush()? (Sorry, this all gets me confused every time.)
>
> I'm just relying on the libfuse documentation there for flush:
>
> "This is called on each close() of the opened file."
Ah, OK.
> and
>
> "NOTE: the name of the method is misleading, since (unlike fsync) the
> filesystem is not forced to flush pending writes. One reason to flush
> data is if the filesystem wants to return write errors during close.
> However, such use is non-portable because POSIX does not require close
> to wait for delayed I/O to complete."
>
>> (Though I do think .fsync should both flush and drain.)
>>
>>> Hmm, or maybe actually for both? We usually do bdrv_flush() during
>>> close, so it would be consistent to do the same here. It's our last
>>> chance to report an error to the user before the file is closed.
>>
>> I don’t understand what you mean. What is “the same”? Closing the
>> image? Or indeed having .flush() be implemented with blk_flush()?
>
> Implementing .flush(), which will be called when the image is closed,
> with blk_flush().
>
> And still doing blk_flush() for .fsync, of course.
OK.
>> Do you mean that other parties may do the same as qemu does, i.e.
>> flush files before they are closed, which is why we should anticipate
>> the same and give users a chance to see errors?
>
> I'm not exactly sure what failure of .flush() would look like for users.
> Probably close() failing, so they don't have to do anything special,
> just check their return values.
Checking return values on close()? Sounds special to me. O:)
Max
signature.asc
Description: OpenPGP digital signature
