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 <mre...@redhat.com> > >>>> +/** >>>> + * 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