Il dom 15 dic 2024, 03:53 ronnie sahlberg <[email protected]> ha
scritto:
> Maybe something like this:
> ---
> diff --git a/block/nfs.c b/block/nfs.c
> index 0500f60c08..f768ee0c4b 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -268,11 +268,18 @@ static int coroutine_fn
> nfs_co_preadv(BlockDriverState *bs, int64_t offset,
> NFSRPC task;
>
> nfs_co_init_task(bs, &task);
> - task.iov = iov;
>
> WITH_QEMU_LOCK_GUARD(&client->mutex) {
> +#ifdef LIBNFS_API_V2
> + if (nfs_pread_async(client->context, client->fh,
> + iov->iov[0].iov_base,
> + bytes > iov->iov[0].iov_len ?
> iov->iov[0].iov_len : bytes,
>
Ah, this was not clear—can the buffer be shorter than the command's
transfer length?
Paolo
> + offset, nfs_co_generic_cb, &task) != 0) {
> +#else
> + task.iov = iov;
> if (nfs_pread_async(client->context, client->fh,
> offset, bytes, nfs_co_generic_cb, &task) !=
> 0) {
> +#endif
> return -ENOMEM;
> }
>
> @@ -317,9 +324,15 @@ static int coroutine_fn
> nfs_co_pwritev(BlockDriverState *bs, int64_t offset,
> }
>
> WITH_QEMU_LOCK_GUARD(&client->mutex) {
> +#ifdef LIBNFS_API_V2
> + if (nfs_pwrite_async(client->context, client->fh,
> + buf, bytes, offset,
> + nfs_co_generic_cb, &task) != 0) {
> +#else
> if (nfs_pwrite_async(client->context, client->fh,
> offset, bytes, buf,
> nfs_co_generic_cb, &task) != 0) {
> +#endif
> if (my_buffer) {
> g_free(buf);
> }
> ---
>
> On Sun, 15 Dec 2024 at 12:04, ronnie sahlberg <[email protected]>
> wrote:
> >
> > On Sun, 15 Dec 2024 at 11:58, ronnie sahlberg <[email protected]>
> wrote:
> > >
> > > On Sun, 15 Dec 2024 at 11:33, Stefan Hajnoczi <[email protected]>
> wrote:
> > > >
> > > > On Fri, 13 Dec 2024 at 11:04, Paolo Bonzini <[email protected]>
> wrote:
> > > > >
> > > > > On 12/13/24 16:51, Richard W.M. Jones wrote:
> > > > > > The libnfs asynch API has changed type signature in this new
> version.
> > > > > > This change breaks qemu and it wasn't immediately obvious to me
> how to
> > > > > > fix it. In particular the new API requires a buffer to be
> passed, but
> > > > > > it's unclear what that would be.
> > > > > >
> > > > > > Old signature:
> > > > > >
> > > > > > EXTERN int nfs_pread_async(struct nfs_context *nfs, struct nfsfh
> *nfsfh,
> > > > > > uint64_t offset, uint64_t count,
> nfs_cb cb,
> > > > > > void *private_data);
> > > > > >
> > > > > > New signature:
> > > > > >
> > > > > > EXTERN int nfs_pread_async(struct nfs_context *nfs, struct nfsfh
> *nfsfh,
> > > > > > void *buf, size_t count, uint64_t
> offset,
> > > > > > nfs_cb cb, void *private_data);
> > > > > >
> > > > > > (Similar changes are made to pwrite_async)
> > > > >
> > > > > Looks like the buffer used to be allocated by libnfs and passed to
> > > > > nfs_co_generic_cb:
> > > > >
> > > > > static void
> > > > > nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data,
> > > > > void *private_data)
> > > > >
> > > > > Now it has to be allocated by the application.
> > > > >
> > > > > (upstream commit
> https://github.com/sahlberg/libnfs/commit/5e8f7ce273308)
> > > > >
> > > > > > I guess this needs upstream qemu attention.
> > > > >
> > > > > And should it also block the libnfs update in Fedora, since QEMU
> is a
> > > > > critical package?
> > > >
> > > > Hi Ronnie,
> > > > This libnfs nfs_pread_async() API change breaks the QEMU build. This
> > > > email thread discusses holding off on updating libnfs 5 -> 6 in
> > > > Fedora, but the update has already happened in Homebrew so the QEMU
> > > > macOS CI is now failing:
> > > > https://gitlab.com/qemu-project/qemu/-/jobs/8646124898
> > > >
> > > > Do you plan to release a new libnfs version that restores API
> > > > compatibility or has the ship sailed and applications need to detect
> > > > this at compile time?
> > >
> > > I would prefer not to restore the old API. I had to break the API for
> > > at least nfs_[p]read[_async]
> > > in order to make (almost) zero-copy within the library work for the
> read path.
> > > I already do zero-copy (within the library) for the write path bit did
> > > not do it for the read path until now
> > > due to how hairy it is to do with the several variable length fields
> > > in the headers :-(
> > >
> > > Since I broke the API for read, which will affect every applicatin I
> > > went ahead and fixed a lot of other warts in the
> > > API as well, so that we can not actually cancel pdus that are in
> flight.
> > >
> > > There is a compile-time check that can be made to determine which API
> is use,
> > > this is from fio :
> > >
> > > @@ -157,16 +157,28 @@ static int queue_write(struct fio_libnfs_options
> > > *o, struct io_u *io_u)
> > > {
> > > struct nfs_data *nfs_data = io_u->engine_data;
> > >
> > > +#ifdef LIBNFS_API_V2
> > > + return nfs_pwrite_async(o->context, nfs_data->nfsfh,
> > > + io_u->buf, io_u->buflen, io_u->offset,
> > > + nfs_callback, io_u);
> > > +#else
> > > return nfs_pwrite_async(o->context, nfs_data->nfsfh,
> io_u->offset,
> > > io_u->buflen, io_u->buf, nfs_callback,
> io_u);
> > > +#endif
> > > }
> > > ...
> > > struct nfs_data *nfs_data = io_u->engine_data;
> > >
> > > +#ifdef LIBNFS_API_V2
> > > + return nfs_pread_async(o->context, nfs_data->nfsfh,
> > > + io_u->buf, io_u->buflen, io_u->offset,
> > > + nfs_callback, io_u);
> > > +#else
> > > return nfs_pread_async(o->context, nfs_data->nfsfh,
> io_u->offset,
> > > io_u->buflen, nfs_callback, io_u);
> > > +#endif
> > >
> > >
> > >
> > > Do you want me to send a patch to qemu to add this type of
> > > compile-time conditionals?
> > > Or will you do it? the change as seen in the snippet above is fairly
> trivial.
> >
> > QEMU is very important application.
> > Let me know if doing the compile-time check as above is feasible.
> > If it is not really feasible due to where/when buffers are allocated in
> qemu,
> > please let me know and then I will restore the old API and introduce
> > the zero-copy versions under a new different function.
> >
> >
> > >
> > >
> > > regards
> > > Ronnie Sahlberg
> > >
> > >
> > > >
> > > > Thanks,
> > > > Stefan
>
>
