* [Qemu-devel] [PATCH] 9pfs: fix migration_block leak
@ 2017-03-30 12:27 Li Qiang
2017-03-30 13:25 ` [Qemu-devel] [PATCH for-2.9?] " Eric Blake
0 siblings, 1 reply; 7+ messages in thread
From: Li Qiang @ 2017-03-30 12:27 UTC (permalink / raw)
To: groug; +Cc: qemu-devel, ppandit, Li Qiang
The guest can leave the pdu->s->migration_blocker exists by attach
but not remove a fid. Then if we hot unplug the 9pfs device, the
v9fs_reset() just free the fids, but not free the migration_blocker.
This will leak a memory leak. This patch avoid this.
Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
hw/9pfs/9p.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 48babce..b55c02d 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
free_fid(pdu, fidp);
}
}
+
+ if (pdu->s->migration_blocker) {
+ migrate_del_blocker(pdu->s->migration_blocker);
+ error_free(pdu->s->migration_blocker);
+ pdu->s->migration_blocker = NULL;
+ }
}
#define P9_QID_TYPE_DIR 0x80
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9?] 9pfs: fix migration_block leak
2017-03-30 12:27 [Qemu-devel] [PATCH] 9pfs: fix migration_block leak Li Qiang
@ 2017-03-30 13:25 ` Eric Blake
2017-03-30 15:46 ` Greg Kurz
0 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2017-03-30 13:25 UTC (permalink / raw)
To: Li Qiang, groug; +Cc: Li Qiang, qemu-devel, ppandit
[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]
On 03/30/2017 07:27 AM, Li Qiang wrote:
> The guest can leave the pdu->s->migration_blocker exists by attach
s/exists/in place/
s/attach/attaching/
> but not remove a fid. Then if we hot unplug the 9pfs device, the
s/remove/removing/
> v9fs_reset() just free the fids, but not free the migration_blocker.
> This will leak a memory leak. This patch avoid this.
s/leak a/cause a/
s/avoid/avoids/
>
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
> hw/9pfs/9p.c | 6 ++++++
> 1 file changed, 6 insertions(+)
Probably worth including in 2.9 as a bug fix.
Reviewed-by: Eric Blake <eblake@redhat.com>
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 48babce..b55c02d 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> free_fid(pdu, fidp);
> }
> }
> +
> + if (pdu->s->migration_blocker) {
> + migrate_del_blocker(pdu->s->migration_blocker);
> + error_free(pdu->s->migration_blocker);
> + pdu->s->migration_blocker = NULL;
> + }
> }
>
> #define P9_QID_TYPE_DIR 0x80
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9?] 9pfs: fix migration_block leak
2017-03-30 13:25 ` [Qemu-devel] [PATCH for-2.9?] " Eric Blake
@ 2017-03-30 15:46 ` Greg Kurz
2017-03-31 1:26 ` Li Qiang
0 siblings, 1 reply; 7+ messages in thread
From: Greg Kurz @ 2017-03-30 15:46 UTC (permalink / raw)
To: Eric Blake; +Cc: Li Qiang, Li Qiang, qemu-devel, ppandit
[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]
On Thu, 30 Mar 2017 08:25:25 -0500
Eric Blake <eblake@redhat.com> wrote:
> On 03/30/2017 07:27 AM, Li Qiang wrote:
> > The guest can leave the pdu->s->migration_blocker exists by attach
>
> s/exists/in place/
> s/attach/attaching/
>
> > but not remove a fid. Then if we hot unplug the 9pfs device, the
>
In theory you're right, but the current 9p client in linux won't let you hot
unplug the device unless you unmount the 9p share first, hence freeing the
blocker.
> s/remove/removing/
>
> > v9fs_reset() just free the fids, but not free the migration_blocker.
> > This will leak a memory leak. This patch avoid this.
I had a similar issue sitting my TODO list for quite a time: the blocker
survives a system_reset. It doesn't cause a memory leak but it prevents
migration until the guest mounts/unmounts the 9p share again.
This boils down to virtfs_reset() calling free_fid() instead of put_fid() IIRC.
>
> s/leak a/cause a/
> s/avoid/avoids/
>
> >
> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > ---
> > hw/9pfs/9p.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
>
> Probably worth including in 2.9 as a bug fix.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 48babce..b55c02d 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> > free_fid(pdu, fidp);
> > }
> > }
> > +
> > + if (pdu->s->migration_blocker) {
> > + migrate_del_blocker(pdu->s->migration_blocker);
> > + error_free(pdu->s->migration_blocker);
> > + pdu->s->migration_blocker = NULL;
> > + }
I'd prefer to drain all PDUs in virtfs_reset() and have the loop above
to call put_fid() instead of free_fid(). If this isn't doable for 2.9,
I'll apply this patch with a comment.
> > }
> >
> > #define P9_QID_TYPE_DIR 0x80
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9?] 9pfs: fix migration_block leak
2017-03-30 15:46 ` Greg Kurz
@ 2017-03-31 1:26 ` Li Qiang
2017-03-31 7:07 ` Greg Kurz
0 siblings, 1 reply; 7+ messages in thread
From: Li Qiang @ 2017-03-31 1:26 UTC (permalink / raw)
To: Greg Kurz; +Cc: Eric Blake, Li Qiang, Qemu Developers, P J P
Hello,
2017-03-30 23:46 GMT+08:00 Greg Kurz <groug@kaod.org>:
> On Thu, 30 Mar 2017 08:25:25 -0500
> Eric Blake <eblake@redhat.com> wrote:
>
> > On 03/30/2017 07:27 AM, Li Qiang wrote:
> > > The guest can leave the pdu->s->migration_blocker exists by attach
> >
> > s/exists/in place/
> > s/attach/attaching/
> >
>
Eric,
Thanks for pointing my mistakes!
> > > but not remove a fid. Then if we hot unplug the 9pfs device, the
> >
>
> In theory you're right, but the current 9p client in linux won't let you
> hot
> unplug the device unless you unmount the 9p share first, hence freeing the
> blocker.
>
>
I think we should consider every possible situation.
> > s/remove/removing/
> >
> > > v9fs_reset() just free the fids, but not free the migration_blocker.
> > > This will leak a memory leak. This patch avoid this.
>
> I had a similar issue sitting my TODO list for quite a time: the blocker
> survives a system_reset. It doesn't cause a memory leak but it prevents
> migration until the guest mounts/unmounts the 9p share again.
>
> This boils down to virtfs_reset() calling free_fid() instead of put_fid()
> IIRC.
>
> >
> > s/leak a/cause a/
> > s/avoid/avoids/
> >
> > >
> > > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > > ---
> > > hw/9pfs/9p.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> >
> > Probably worth including in 2.9 as a bug fix.
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> > >
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 48babce..b55c02d 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU
> *pdu)
> > > free_fid(pdu, fidp);
> > > }
> > > }
> > > +
> > > + if (pdu->s->migration_blocker) {
> > > + migrate_del_blocker(pdu->s->migration_blocker);
> > > + error_free(pdu->s->migration_blocker);
> > > + pdu->s->migration_blocker = NULL;
> > > + }
>
> I'd prefer to drain all PDUs in virtfs_reset() and have the loop above
> to call put_fid() instead of free_fid(). If this isn't doable for 2.9,
> I'll apply this patch with a comment.
>
>
Yes, I have considered to use put_fid() to fix this. But I'm not sure the
'fidp->ref' is at most 1 in virtfs_reset() function(I think it is).
IIUC I think omit the 'else' branch, and call put_fid() directly like this.
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 48babce..ae97e79 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -544,9 +544,8 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
if (fidp->ref) {
fidp->clunked = 1;
- } else {
- free_fid(pdu, fidp);
}
+ put_fid(pdu, fidp);
}
}
If you agree, I will send a formal patch.
> > > }
> > >
> > > #define P9_QID_TYPE_DIR 0x80
> > >
> >
>
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9?] 9pfs: fix migration_block leak
2017-03-31 1:26 ` Li Qiang
@ 2017-03-31 7:07 ` Greg Kurz
2017-03-31 8:01 ` Li Qiang
0 siblings, 1 reply; 7+ messages in thread
From: Greg Kurz @ 2017-03-31 7:07 UTC (permalink / raw)
To: Li Qiang; +Cc: Eric Blake, Li Qiang, Qemu Developers, P J P
[-- Attachment #1: Type: text/plain, Size: 3554 bytes --]
On Fri, 31 Mar 2017 09:26:35 +0800
Li Qiang <liq3ea@gmail.com> wrote:
> Hello,
>
> 2017-03-30 23:46 GMT+08:00 Greg Kurz <groug@kaod.org>:
>
> > On Thu, 30 Mar 2017 08:25:25 -0500
> > Eric Blake <eblake@redhat.com> wrote:
> >
> > > On 03/30/2017 07:27 AM, Li Qiang wrote:
> > > > The guest can leave the pdu->s->migration_blocker exists by attach
> > >
> > > s/exists/in place/
> > > s/attach/attaching/
> > >
> >
>
> Eric,
> Thanks for pointing my mistakes!
>
>
> > > > but not remove a fid. Then if we hot unplug the 9pfs device, the
> > >
> >
> > In theory you're right, but the current 9p client in linux won't let you
> > hot
> > unplug the device unless you unmount the 9p share first, hence freeing the
> > blocker.
> >
> >
> I think we should consider every possible situation.
>
>
> > > s/remove/removing/
> > >
> > > > v9fs_reset() just free the fids, but not free the migration_blocker.
> > > > This will leak a memory leak. This patch avoid this.
> >
> > I had a similar issue sitting my TODO list for quite a time: the blocker
> > survives a system_reset. It doesn't cause a memory leak but it prevents
> > migration until the guest mounts/unmounts the 9p share again.
> >
> > This boils down to virtfs_reset() calling free_fid() instead of put_fid()
> > IIRC.
> >
> > >
> > > s/leak a/cause a/
> > > s/avoid/avoids/
> > >
> > > >
> > > > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > > > ---
> > > > hw/9pfs/9p.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > >
> > > Probably worth including in 2.9 as a bug fix.
> > >
> > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > >
> > > >
> > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > index 48babce..b55c02d 100644
> > > > --- a/hw/9pfs/9p.c
> > > > +++ b/hw/9pfs/9p.c
> > > > @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU
> > *pdu)
> > > > free_fid(pdu, fidp);
> > > > }
> > > > }
> > > > +
> > > > + if (pdu->s->migration_blocker) {
> > > > + migrate_del_blocker(pdu->s->migration_blocker);
> > > > + error_free(pdu->s->migration_blocker);
> > > > + pdu->s->migration_blocker = NULL;
> > > > + }
> >
> > I'd prefer to drain all PDUs in virtfs_reset() and have the loop above
> > to call put_fid() instead of free_fid(). If this isn't doable for 2.9,
> > I'll apply this patch with a comment.
> >
> >
> Yes, I have considered to use put_fid() to fix this. But I'm not sure the
> 'fidp->ref' is at most 1 in virtfs_reset() function(I think it is).
>
And indeed, if the fid is involved in an I/O then its ref will be != 0.
> IIUC I think omit the 'else' branch, and call put_fid() directly like this.
>
This won't work: we must ensure that fidp->ref reaches zero (ie, drain
all PDUs), then we can fidp->ref++ (ie, get a ref on the fid) and call
put_fid(), which will drop the last ref of the fid and clear the blocker
if this is the root fid.
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 48babce..ae97e79 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -544,9 +544,8 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
>
> if (fidp->ref) {
> fidp->clunked = 1;
> - } else {
> - free_fid(pdu, fidp);
> }
> + put_fid(pdu, fidp);
> }
> }
>
>
> If you agree, I will send a formal patch.
>
>
>
> > > > }
> > > >
> > > > #define P9_QID_TYPE_DIR 0x80
> > > >
> > >
> >
> >
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9?] 9pfs: fix migration_block leak
2017-03-31 7:07 ` Greg Kurz
@ 2017-03-31 8:01 ` Li Qiang
2017-03-31 8:37 ` Greg Kurz
0 siblings, 1 reply; 7+ messages in thread
From: Li Qiang @ 2017-03-31 8:01 UTC (permalink / raw)
To: Greg Kurz; +Cc: Eric Blake, Li Qiang, Qemu Developers, P J P
2017-03-31 15:07 GMT+08:00 Greg Kurz <groug@kaod.org>:
> On Fri, 31 Mar 2017 09:26:35 +0800
> Li Qiang <liq3ea@gmail.com> wrote:
>
> > Hello,
> >
> > 2017-03-30 23:46 GMT+08:00 Greg Kurz <groug@kaod.org>:
> >
> > > On Thu, 30 Mar 2017 08:25:25 -0500
> > > Eric Blake <eblake@redhat.com> wrote:
> > >
> > > > On 03/30/2017 07:27 AM, Li Qiang wrote:
> > > > > The guest can leave the pdu->s->migration_blocker exists by attach
> > > >
> > > > s/exists/in place/
> > > > s/attach/attaching/
> > > >
> > >
> >
> > Eric,
> > Thanks for pointing my mistakes!
> >
> >
> > > > > but not remove a fid. Then if we hot unplug the 9pfs device, the
> > > >
> > >
> > > In theory you're right, but the current 9p client in linux won't let
> you
> > > hot
> > > unplug the device unless you unmount the 9p share first, hence freeing
> the
> > > blocker.
> > >
> > >
> > I think we should consider every possible situation.
> >
> >
> > > > s/remove/removing/
> > > >
> > > > > v9fs_reset() just free the fids, but not free the
> migration_blocker.
> > > > > This will leak a memory leak. This patch avoid this.
> > >
> > > I had a similar issue sitting my TODO list for quite a time: the
> blocker
> > > survives a system_reset. It doesn't cause a memory leak but it prevents
> > > migration until the guest mounts/unmounts the 9p share again.
> > >
> > > This boils down to virtfs_reset() calling free_fid() instead of
> put_fid()
> > > IIRC.
> > >
> > > >
> > > > s/leak a/cause a/
> > > > s/avoid/avoids/
> > > >
> > > > >
> > > > > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > > > > ---
> > > > > hw/9pfs/9p.c | 6 ++++++
> > > > > 1 file changed, 6 insertions(+)
> > > >
> > > > Probably worth including in 2.9 as a bug fix.
> > > >
> > > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > >
> > > > >
> > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > > index 48babce..b55c02d 100644
> > > > > --- a/hw/9pfs/9p.c
> > > > > +++ b/hw/9pfs/9p.c
> > > > > @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU
> > > *pdu)
> > > > > free_fid(pdu, fidp);
> > > > > }
> > > > > }
> > > > > +
> > > > > + if (pdu->s->migration_blocker) {
> > > > > + migrate_del_blocker(pdu->s->migration_blocker);
> > > > > + error_free(pdu->s->migration_blocker);
> > > > > + pdu->s->migration_blocker = NULL;
> > > > > + }
> > >
> > > I'd prefer to drain all PDUs in virtfs_reset() and have the loop above
> > > to call put_fid() instead of free_fid(). If this isn't doable for 2.9,
> > > I'll apply this patch with a comment.
> > >
> > >
> > Yes, I have considered to use put_fid() to fix this. But I'm not sure the
> > 'fidp->ref' is at most 1 in virtfs_reset() function(I think it is).
> >
>
> And indeed, if the fid is involved in an I/O then its ref will be != 0.
>
> > IIUC I think omit the 'else' branch, and call put_fid() directly like
> this.
> >
>
> This won't work: we must ensure that fidp->ref reaches zero (ie, drain
> all PDUs), then we can fidp->ref++ (ie, get a ref on the fid) and call
> put_fid(), which will drop the last ref of the fid and clear the blocker
> if this is the root fid.
>
>
Right, but how can we ensure we have drained all PDUs? Any idea?
Thanks.
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 48babce..ae97e79 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -544,9 +544,8 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> >
> > if (fidp->ref) {
> > fidp->clunked = 1;
> > - } else {
> > - free_fid(pdu, fidp);
> > }
> > + put_fid(pdu, fidp);
> > }
> > }
> >
> >
> > If you agree, I will send a formal patch.
> >
> >
> >
> > > > > }
> > > > >
> > > > > #define P9_QID_TYPE_DIR 0x80
> > > > >
> > > >
> > >
> > >
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9?] 9pfs: fix migration_block leak
2017-03-31 8:01 ` Li Qiang
@ 2017-03-31 8:37 ` Greg Kurz
0 siblings, 0 replies; 7+ messages in thread
From: Greg Kurz @ 2017-03-31 8:37 UTC (permalink / raw)
To: Li Qiang; +Cc: Eric Blake, Li Qiang, Qemu Developers, P J P
[-- Attachment #1: Type: text/plain, Size: 4349 bytes --]
On Fri, 31 Mar 2017 16:01:15 +0800
Li Qiang <liq3ea@gmail.com> wrote:
> 2017-03-31 15:07 GMT+08:00 Greg Kurz <groug@kaod.org>:
>
> > On Fri, 31 Mar 2017 09:26:35 +0800
> > Li Qiang <liq3ea@gmail.com> wrote:
> >
> > > Hello,
> > >
> > > 2017-03-30 23:46 GMT+08:00 Greg Kurz <groug@kaod.org>:
> > >
> > > > On Thu, 30 Mar 2017 08:25:25 -0500
> > > > Eric Blake <eblake@redhat.com> wrote:
> > > >
> > > > > On 03/30/2017 07:27 AM, Li Qiang wrote:
> > > > > > The guest can leave the pdu->s->migration_blocker exists by attach
> > > > >
> > > > > s/exists/in place/
> > > > > s/attach/attaching/
> > > > >
> > > >
> > >
> > > Eric,
> > > Thanks for pointing my mistakes!
> > >
> > >
> > > > > > but not remove a fid. Then if we hot unplug the 9pfs device, the
> > > > >
> > > >
> > > > In theory you're right, but the current 9p client in linux won't let
> > you
> > > > hot
> > > > unplug the device unless you unmount the 9p share first, hence freeing
> > the
> > > > blocker.
> > > >
> > > >
> > > I think we should consider every possible situation.
> > >
> > >
> > > > > s/remove/removing/
> > > > >
> > > > > > v9fs_reset() just free the fids, but not free the
> > migration_blocker.
> > > > > > This will leak a memory leak. This patch avoid this.
> > > >
> > > > I had a similar issue sitting my TODO list for quite a time: the
> > blocker
> > > > survives a system_reset. It doesn't cause a memory leak but it prevents
> > > > migration until the guest mounts/unmounts the 9p share again.
> > > >
> > > > This boils down to virtfs_reset() calling free_fid() instead of
> > put_fid()
> > > > IIRC.
> > > >
> > > > >
> > > > > s/leak a/cause a/
> > > > > s/avoid/avoids/
> > > > >
> > > > > >
> > > > > > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > > > > > ---
> > > > > > hw/9pfs/9p.c | 6 ++++++
> > > > > > 1 file changed, 6 insertions(+)
> > > > >
> > > > > Probably worth including in 2.9 as a bug fix.
> > > > >
> > > > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > > >
> > > > > >
> > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > > > > index 48babce..b55c02d 100644
> > > > > > --- a/hw/9pfs/9p.c
> > > > > > +++ b/hw/9pfs/9p.c
> > > > > > @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU
> > > > *pdu)
> > > > > > free_fid(pdu, fidp);
> > > > > > }
> > > > > > }
> > > > > > +
> > > > > > + if (pdu->s->migration_blocker) {
> > > > > > + migrate_del_blocker(pdu->s->migration_blocker);
> > > > > > + error_free(pdu->s->migration_blocker);
> > > > > > + pdu->s->migration_blocker = NULL;
> > > > > > + }
> > > >
> > > > I'd prefer to drain all PDUs in virtfs_reset() and have the loop above
> > > > to call put_fid() instead of free_fid(). If this isn't doable for 2.9,
> > > > I'll apply this patch with a comment.
> > > >
> > > >
> > > Yes, I have considered to use put_fid() to fix this. But I'm not sure the
> > > 'fidp->ref' is at most 1 in virtfs_reset() function(I think it is).
> > >
> >
> > And indeed, if the fid is involved in an I/O then its ref will be != 0.
> >
> > > IIUC I think omit the 'else' branch, and call put_fid() directly like
> > this.
> > >
> >
> > This won't work: we must ensure that fidp->ref reaches zero (ie, drain
> > all PDUs), then we can fidp->ref++ (ie, get a ref on the fid) and call
> > put_fid(), which will drop the last ref of the fid and clear the blocker
> > if this is the root fid.
> >
> >
> Right, but how can we ensure we have drained all PDUs? Any idea?
>
Yep, I'll send patches shorty and Cc you.
> Thanks.
>
>
>
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 48babce..ae97e79 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -544,9 +544,8 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> > >
> > > if (fidp->ref) {
> > > fidp->clunked = 1;
> > > - } else {
> > > - free_fid(pdu, fidp);
> > > }
> > > + put_fid(pdu, fidp);
> > > }
> > > }
> > >
> > >
> > > If you agree, I will send a formal patch.
> > >
> > >
> > >
> > > > > > }
> > > > > >
> > > > > > #define P9_QID_TYPE_DIR 0x80
> > > > > >
> > > > >
> > > >
> > > >
> >
> >
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-31 8:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 12:27 [Qemu-devel] [PATCH] 9pfs: fix migration_block leak Li Qiang
2017-03-30 13:25 ` [Qemu-devel] [PATCH for-2.9?] " Eric Blake
2017-03-30 15:46 ` Greg Kurz
2017-03-31 1:26 ` Li Qiang
2017-03-31 7:07 ` Greg Kurz
2017-03-31 8:01 ` Li Qiang
2017-03-31 8:37 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.