| Message ID | 1490876834-61392-1-git-send-email-liqiang6-s@360.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 > > >
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
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(+)