On Fri, Mar 01, 2024 at 05:50:24PM +0000, Daniel P. Berrangé wrote: > On Fri, Mar 01, 2024 at 11:17:10PM +0530, Prasad Pandit wrote: > > Hello Petr,
Hey Prasad! Thanks for taking a look. > > > > On Fri, 1 Mar 2024 at 14:46, <pet...@redhat.com> wrote: > > > + * An explicitly close() on the channel here is normally not > > > > explicitly -> explicit > > > > > + * required, but can be helpful for "file:" iochannels, where it > > > + * will include an fdatasync() to make sure the data is flushed > > > to > > > + * the disk backend. > > > > * an fdatasync() -> fdatasync() I'll fix both when apply. > > > > * qio_channel_close > > -> ioc_klass->io_close = qio_channel_file_close; > > -> qemu_close(fioc->fd) > > -> close(fd); > > > > It does not seem to call fdatasync() before close(fd); > > > > - qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, ...) > > The documented behaviour reliant on another pending patch: > > https://lists.nongnu.org/archive/html/qemu-devel/2024-02/msg07046.html Rightfully as Dan helped to answer. While for the other comment section in the same patch it relies on the other patch: https://lore.kernel.org/all/20240229153017.2221-20-faro...@suse.de/ > > > > > Maybe the qio_channel..() calls above should include the 'O_DSYNC' > > flag as well? But that will do fdatasync() work at each write(2) call > > I think, not sure if that is okay. > > > > > > > > + * > > > + * The object_unref() cannot guarantee that because: (1) > > > finalize() > > > + * of the iochannel is only triggered on the last reference, and > > > + * it's not guaranteed that we always hold the last refcount when > > > + * reaching here, and, (2) even if finalize() is invoked, it only > > > + * does a close(fd) without data flush. > > > + */ > > > > * object_unref > > -> object_finalize > > -> object_deinit > > -> type->instance_finalize > > -> qio_channel_file_finalize > > -> qemu_close(ioc->fd); > > > > * I hope I'm looking at the right code here. (Sorry if I'm not) Yes I think you're looking at the right path, it's just that the relevant patches haven't yet landed upstream but is planned. I normally use "Based-on" tag for such patch that has a dependency outside master, like: Based-on: <20240229153017.2221-1-faro...@suse.de> I believe many others on the qemu list do the same. I think the rational is this will be both recognized by human beings and also by patchew, e.g. patchew will report a good apply status here: https://patchew.org/QEMU/20240301091524.39900-1-pet...@redhat.com/ And in the same patch if you fetch the tree patchew provided: git fetch https://github.com/patchew-project/qemu tags/patchew/20240301091524.39900-1-pet...@redhat.com You can also directly fetch the whole tree with this patch applied correctly on top of the dependency series. Personally I don't use patchew, but I kept that habit to declare patch dependencies just in case it'll help others who use it (e.g., I think patchew has other review tools like version comparisons, which I also don't use myself). Thanks, -- Peter Xu
