Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update
"Dr. David Alan Gilbert (git)"wrote: > From: "Dr. David Alan Gilbert" > > My fix (84e7b80a) replaced the last_sent_block update that I'd > removed earlier; however it was too aggressive in the xbzrle case. > > save_xbzrle_page might return '0' to mean that the page didn't > need sending since it was the same as the last sent version; > in this case we can't update 'last_sent_block' since we didn't > actually send it. > > Symptom: 'Illegal RAM offset 1018000' as we try and send a page > to the wrong RAMBlock; potentially that could be a data > corruption if you were really unlucky. > > Fixes: 84e7b80a05c0c44b90533c6cd2f1db5c932ccf77 > > Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela
Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update
Peter Maydellwrote: D> On 10 December 2015 at 16:31, Dr. David Alan Gilbert (git) > wrote: >> From: "Dr. David Alan Gilbert" >> >> My fix (84e7b80a) replaced the last_sent_block update that I'd >> removed earlier; however it was too aggressive in the xbzrle case. >> >> save_xbzrle_page might return '0' to mean that the page didn't >> need sending since it was the same as the last sent version; >> in this case we can't update 'last_sent_block' since we didn't >> actually send it. >> >> Symptom: 'Illegal RAM offset 1018000' as we try and send a page >> to the wrong RAMBlock; potentially that could be a data >> corruption if you were really unlucky. >> >> Fixes: 84e7b80a05c0c44b90533c6cd2f1db5c932ccf77 >> >> Signed-off-by: Dr. David Alan Gilbert >> --- >> migration/ram.c | 11 ++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 1eb155a..0490f00 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -716,6 +716,9 @@ static int save_zero_page(QEMUFile *f, RAMBlock *block, >> ram_addr_t offset, >> * ram_save_page: Send the given page to the stream >> * >> * Returns: Number of pages written. >> + * < 0 - error >> + * >=0 - Number of pages written - this might legally be 0 >> + *if xbzrle noticed the page was the same. >> * >> * @f: QEMUFile where to send the data >> * @block: block that contains the page we want to send >> @@ -1249,7 +1252,13 @@ static int ram_save_target_page(MigrationState *ms, >> QEMUFile *f, >> if (unsentmap) { >> clear_bit(dirty_ram_abs >> TARGET_PAGE_BITS, unsentmap); >> } >> -last_sent_block = block; >> +/* Only update last_sent_block if a block was actually sent; xbzrle >> + * might have decided the page was identical so didn't bother >> writing >> + * to the stream. >> + */ >> +if (res > 0) { >> +last_sent_block = block; >> +} >> } >> >> return res; > > This sounds like we should probably put this into 2.5; I'm happy > to do so if it gets review by tomorrow afternoon and Juan/Amit > agree. Yeap, did the review by. Do you want a pull request, or just pick it directly?
Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update
On 11 December 2015 at 12:52, Peter Maydellwrote: > On 11 December 2015 at 12:25, Juan Quintela wrote: >> Peter Maydell wrote: >>> This sounds like we should probably put this into 2.5; I'm happy >>> to do so if it gets review by tomorrow afternoon and Juan/Amit >>> agree. >> >> Yeap, did the review by. Do you want a pull request, or just pick it >> directly? > > I'll just apply it directly. Now applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update
On 11 December 2015 at 12:25, Juan Quintelawrote: > Peter Maydell wrote: >> This sounds like we should probably put this into 2.5; I'm happy >> to do so if it gets review by tomorrow afternoon and Juan/Amit >> agree. > > Yeap, did the review by. Do you want a pull request, or just pick it > directly? I'll just apply it directly. thanks -- PMM
Re: [Qemu-devel] [PATCH] Fix xbzrle vs last_sent_block update
On 10 December 2015 at 16:31, Dr. David Alan Gilbert (git)wrote: > From: "Dr. David Alan Gilbert" > > My fix (84e7b80a) replaced the last_sent_block update that I'd > removed earlier; however it was too aggressive in the xbzrle case. > > save_xbzrle_page might return '0' to mean that the page didn't > need sending since it was the same as the last sent version; > in this case we can't update 'last_sent_block' since we didn't > actually send it. > > Symptom: 'Illegal RAM offset 1018000' as we try and send a page > to the wrong RAMBlock; potentially that could be a data > corruption if you were really unlucky. > > Fixes: 84e7b80a05c0c44b90533c6cd2f1db5c932ccf77 > > Signed-off-by: Dr. David Alan Gilbert > --- > migration/ram.c | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 1eb155a..0490f00 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -716,6 +716,9 @@ static int save_zero_page(QEMUFile *f, RAMBlock *block, > ram_addr_t offset, > * ram_save_page: Send the given page to the stream > * > * Returns: Number of pages written. > + * < 0 - error > + * >=0 - Number of pages written - this might legally be 0 > + *if xbzrle noticed the page was the same. > * > * @f: QEMUFile where to send the data > * @block: block that contains the page we want to send > @@ -1249,7 +1252,13 @@ static int ram_save_target_page(MigrationState *ms, > QEMUFile *f, > if (unsentmap) { > clear_bit(dirty_ram_abs >> TARGET_PAGE_BITS, unsentmap); > } > -last_sent_block = block; > +/* Only update last_sent_block if a block was actually sent; xbzrle > + * might have decided the page was identical so didn't bother writing > + * to the stream. > + */ > +if (res > 0) { > +last_sent_block = block; > +} > } > > return res; This sounds like we should probably put this into 2.5; I'm happy to do so if it gets review by tomorrow afternoon and Juan/Amit agree. thanks -- PMM