| Message ID | 1449765106-6528-1-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10 December 2015 at 16:31, Dr. David Alan Gilbert (git) <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > 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 <dgilbert@redhat.com> > --- > 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
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > 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 <dgilbert@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com>
Peter Maydell <peter.maydell@linaro.org> wrote: D> On 10 December 2015 at 16:31, Dr. David Alan Gilbert (git) > <dgilbert@redhat.com> wrote: >> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> 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 <dgilbert@redhat.com> >> --- >> 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?
On 11 December 2015 at 12:25, Juan Quintela <quintela@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> 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
On 11 December 2015 at 12:52, Peter Maydell <peter.maydell@linaro.org> wrote: > On 11 December 2015 at 12:25, Juan Quintela <quintela@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> 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
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;