Re: [PATCH] migration/multifd: Document two places for mapped-ram

[Peter Xu <peterx@redhat.com>]

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, <peterx@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-farosas@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-farosas@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-peterx@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-peterx@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