| Message ID | 20170919115055.19278-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 19, 2017 at 01:50:55PM +0200, Juergen Gross wrote: > Trying to call xengnttab_set_max_grants() with the same file handle > might fail on some kernels, as this operation is allowed only once. > > This is a problem for the qdisk backend as blk_connect() can be > called multiple times for a domain, e.g. in case grub-xen is being > used to boot it. > > So instead of letting the generic backend code open the gnttab device > do it in blk_connect() and close it again in blk_disconnect. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > hw/block/xen_disk.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 6632746250..7cff8863cb 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -1220,6 +1220,12 @@ static int blk_connect(struct XenDevice *xendev) > /* Add on the number needed for the ring pages */ > max_grants += blkdev->nr_ring_ref; > > + blkdev->xendev.gnttabdev = xengnttab_open(NULL, 0); > + if (blkdev->xendev.gnttabdev == NULL) { > + xen_pv_printf(xendev, 0, "xengnttab_open failed: %s\n", > + strerror(errno)); > + return -1; > + } > if (xengnttab_set_max_grants(blkdev->xendev.gnttabdev, max_grants)) { > xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n", > strerror(errno)); > @@ -1327,6 +1333,11 @@ static void blk_disconnect(struct XenDevice *xendev) > } > blkdev->feature_persistent = false; > } > + > + if (blkdev->xendev.gnttabdev) { > + xengnttab_close(blkdev->xendev.gnttabdev); > + blkdev->xendev.gnttabdev = NULL; > + } I think blk_disconnect needs to be called from blk_free in case where the gnttabdev is not closed (like it is done when blk or the sring are not cleared, in blk_free). > } > static int blk_free(struct XenDevice *xendev) > @@ -1363,7 +1374,6 @@ static void blk_event(struct XenDevice *xendev) > > struct XenDevOps xen_blkdev_ops = { > .size = sizeof(struct XenBlkDev), > - .flags = DEVOPS_FLAG_NEED_GNTDEV, > .alloc = blk_alloc, > .init = blk_init, > .initialise = blk_connect, > -- > 2.12.3 >
On 20/09/17 17:00, Anthony PERARD wrote: > On Tue, Sep 19, 2017 at 01:50:55PM +0200, Juergen Gross wrote: >> Trying to call xengnttab_set_max_grants() with the same file handle >> might fail on some kernels, as this operation is allowed only once. >> >> This is a problem for the qdisk backend as blk_connect() can be >> called multiple times for a domain, e.g. in case grub-xen is being >> used to boot it. >> >> So instead of letting the generic backend code open the gnttab device >> do it in blk_connect() and close it again in blk_disconnect. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> hw/block/xen_disk.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c >> index 6632746250..7cff8863cb 100644 >> --- a/hw/block/xen_disk.c >> +++ b/hw/block/xen_disk.c >> @@ -1220,6 +1220,12 @@ static int blk_connect(struct XenDevice *xendev) >> /* Add on the number needed for the ring pages */ >> max_grants += blkdev->nr_ring_ref; >> >> + blkdev->xendev.gnttabdev = xengnttab_open(NULL, 0); >> + if (blkdev->xendev.gnttabdev == NULL) { >> + xen_pv_printf(xendev, 0, "xengnttab_open failed: %s\n", >> + strerror(errno)); >> + return -1; >> + } >> if (xengnttab_set_max_grants(blkdev->xendev.gnttabdev, max_grants)) { >> xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n", >> strerror(errno)); >> @@ -1327,6 +1333,11 @@ static void blk_disconnect(struct XenDevice *xendev) >> } >> blkdev->feature_persistent = false; >> } >> + >> + if (blkdev->xendev.gnttabdev) { >> + xengnttab_close(blkdev->xendev.gnttabdev); >> + blkdev->xendev.gnttabdev = NULL; >> + } > > I think blk_disconnect needs to be called from blk_free in case where > the gnttabdev is not closed (like it is done when blk or the sring are > not cleared, in blk_free). Right. Just calling it always from blk_free() should do the job. BTW: in practice this wouldn't be necessary as xen_pv_del_xendev() already does the close, but this is just good luck. Better closing it via blk_free() in case xen_pv_del_xendev() adds a test for DEVOPS_FLAG_NEED_GNTDEV. Thanks, Juergen
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c index 6632746250..7cff8863cb 100644 --- a/hw/block/xen_disk.c +++ b/hw/block/xen_disk.c @@ -1220,6 +1220,12 @@ static int blk_connect(struct XenDevice *xendev) /* Add on the number needed for the ring pages */ max_grants += blkdev->nr_ring_ref; + blkdev->xendev.gnttabdev = xengnttab_open(NULL, 0); + if (blkdev->xendev.gnttabdev == NULL) { + xen_pv_printf(xendev, 0, "xengnttab_open failed: %s\n", + strerror(errno)); + return -1; + } if (xengnttab_set_max_grants(blkdev->xendev.gnttabdev, max_grants)) { xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n", strerror(errno)); @@ -1327,6 +1333,11 @@ static void blk_disconnect(struct XenDevice *xendev) } blkdev->feature_persistent = false; } + + if (blkdev->xendev.gnttabdev) { + xengnttab_close(blkdev->xendev.gnttabdev); + blkdev->xendev.gnttabdev = NULL; + } } static int blk_free(struct XenDevice *xendev) @@ -1363,7 +1374,6 @@ static void blk_event(struct XenDevice *xendev) struct XenDevOps xen_blkdev_ops = { .size = sizeof(struct XenBlkDev), - .flags = DEVOPS_FLAG_NEED_GNTDEV, .alloc = blk_alloc, .init = blk_init, .initialise = blk_connect,
Trying to call xengnttab_set_max_grants() with the same file handle might fail on some kernels, as this operation is allowed only once. This is a problem for the qdisk backend as blk_connect() can be called multiple times for a domain, e.g. in case grub-xen is being used to boot it. So instead of letting the generic backend code open the gnttab device do it in blk_connect() and close it again in blk_disconnect. Signed-off-by: Juergen Gross <jgross@suse.com> --- hw/block/xen_disk.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)