Hey Jeremy,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git
for-2.6.32/bug-fixes
It has two bug-fixes in it. The first one is quite specific and it only handles
the case if you run v2.6.32 on an IBM Summit machine. The other fixes an issue
with xen-blkback wherein a barrier request would have been discarded (and an
error
returned) b/c the sector provided via the request was -1. The -1 sector made
vbd_translate return an error (it checked the sector number against the size of
the disk) and it would never go through trying to do a barrier. The second
bug-fix
is also in my devel/xen-blkback-v3.2 upstream tree.
Konrad Rzeszutek Wilk (2):
xen/apic: Provide an ''apic_xen'' to set the override the
apic->[read|write] for all cases.
xen/blkback: When writting barriers set the sector number to zero...
arch/x86/kernel/apic/probe_32.c | 4 ++++
arch/x86/kernel/apic/probe_64.c | 4 ++++
arch/x86/xen/enlighten.c | 26 ++++++++++++++++++++++++++
drivers/xen/blkback/blkback.c | 2 ++
4 files changed, 36 insertions(+)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
>>> On 16.05.11 at 22:35, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > with xen-blkback wherein a barrier request would have been discarded (and an error > returned) b/c the sector provided via the request was -1. The -1 sector made > vbd_translate return an error (it checked the sector number against the size of > the disk) and it would never go through trying to do a barrier. The second bug-fix > is also in my devel/xen-blkback-v3.2 upstream tree.Is this really correct? You appear to assume that BLKIF_OP_WRITE_BARRIER always has no data, but the rest of the code in the driver (and the frontend) doesn''t seem to imply that (see e.g. the check immediately following the switch statement your patch modifies). Hence shouldn''t you clear the sector number only when req->nr_segments is zero? Or alternatively, shouldn''t vbd_translate() simply not fail when req->nr_sects is zero? Additionally, looking at the check in vbd_translate(), wouldn''t you think there ought to be overflow checking for the addition, too? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 17.05.11 at 11:48, "Jan Beulich" <JBeulich@novell.com> wrote: >>>> On 16.05.11 at 22:35, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> with xen-blkback wherein a barrier request would have been discarded (and an > error >> returned) b/c the sector provided via the request was -1. The -1 sector made >> vbd_translate return an error (it checked the sector number against the size > of >> the disk) and it would never go through trying to do a barrier. The second > bug-fix >> is also in my devel/xen-blkback-v3.2 upstream tree. > > Is this really correct? You appear to assume that BLKIF_OP_WRITE_BARRIER > always has no data, but the rest of the code in the driver (and > the frontend) doesn''t seem to imply that (see e.g. the check > immediately following the switch statement your patch modifies). > Hence shouldn''t you clear the sector number only when > req->nr_segments is zero? Or alternatively, shouldn''t > vbd_translate() simply not fail when req->nr_sects is zero? > > Additionally, looking at the check in vbd_translate(), wouldn''t you > think there ought to be overflow checking for the addition, too?Altogether e.g. Subject: xen/blkback: don''t fail empty barrier requests The sector number on empty barrier requests may (will?) be -1, which, given that it''s being treated as unsigned 64-bit quantity, will almost always exceed the actual (virtual) disk''s size. Inspired by Konrad''s "When writting barriers set the sector number to zero...". While at it also add overflow checking to the math in vbd_translate(). Signed-off-by: Jan Beulich <jbeulich@novell.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- a/drivers/xen/blkback/vbd.c +++ b/drivers/xen/blkback/vbd.c @@ -114,8 +114,14 @@ int vbd_translate(struct phys_req *req, if (vbd->bdev == NULL) goto out; - if (unlikely((req->sector_number + req->nr_sects) > vbd_sz(vbd))) - goto out; + if (likely(req->nr_sects)) { + blkif_sector_t end = req->sector_number + req->nr_sects; + + if (unlikely(end < req->sector_number)) + goto out; + if (unlikely(end > vbd_sz(vbd))) + goto out; + } req->dev = vbd->pdevice; req->bdev = vbd->bdev; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-May-17 14:16 UTC
Re: [Xen-devel] [GIT PULL] for-2.6.32/bug-fixes
On Tue, May 17, 2011 at 10:48:00AM +0100, Jan Beulich wrote:> >>> On 16.05.11 at 22:35, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > with xen-blkback wherein a barrier request would have been discarded (and an error > > returned) b/c the sector provided via the request was -1. The -1 sector made > > vbd_translate return an error (it checked the sector number against the size of > > the disk) and it would never go through trying to do a barrier. The second bug-fix > > is also in my devel/xen-blkback-v3.2 upstream tree. > > Is this really correct? You appear to assume that BLKIF_OP_WRITE_BARRIER > always has no data, but the rest of the code in the driver (and > the frontend) doesn''t seem to imply that (see e.g. the check > immediately following the switch statement your patch modifies).That is correct. Look at the end of the code logic (this is from the 2.6.18 hg tree): 528 if (!bio) { 529 BUG_ON(operation != WRITE_BARRIER); 530 bio = bio_alloc(GFP_KERNEL, 0); 531 if (unlikely(bio == NULL)) 532 goto fail_put_bio; 533 534 bio->bi_bdev = preq.bdev; 535 bio->bi_private = pending_req; 536 bio->bi_end_io = end_block_io_op; 537 bio->bi_sector = -1; 538 } No attaching of data to the barrier.> Hence shouldn''t you clear the sector number only when > req->nr_segments is zero? Or alternatively, shouldn''tWe could do that too.> vbd_translate() simply not fail when req->nr_sects is zero?It does not fail when req->nr_sects is zero. It fails when it is -1.> > Additionally, looking at the check in vbd_translate(), wouldn''t you > think there ought to be overflow checking for the addition, too?Sure, could add that in. Albeit it seems incorrect to do it in that function. It checks to see if the sector is correct, and -1 is definitly wrong.> > Jan_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 17.05.11 at 16:16, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Tue, May 17, 2011 at 10:48:00AM +0100, Jan Beulich wrote: >> >>> On 16.05.11 at 22:35, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> > with xen-blkback wherein a barrier request would have been discarded (and an > error >> > returned) b/c the sector provided via the request was -1. The -1 sector made >> > vbd_translate return an error (it checked the sector number against the > size of >> > the disk) and it would never go through trying to do a barrier. The second > bug-fix >> > is also in my devel/xen-blkback-v3.2 upstream tree. >> >> Is this really correct? You appear to assume that BLKIF_OP_WRITE_BARRIER >> always has no data, but the rest of the code in the driver (and >> the frontend) doesn''t seem to imply that (see e.g. the check >> immediately following the switch statement your patch modifies). > > That is correct. Look at the end of the code logic (this is from the 2.6.18 > hg tree): > > 528 if (!bio) { > 529 BUG_ON(operation != WRITE_BARRIER); > 530 bio = bio_alloc(GFP_KERNEL, 0); > 531 if (unlikely(bio == NULL)) > 532 goto fail_put_bio; > 533 > 534 bio->bi_bdev = preq.bdev; > 535 bio->bi_private = pending_req; > 536 bio->bi_end_io = end_block_io_op; > 537 bio->bi_sector = -1; > 538 } > > No attaching of data to the barrier.Sure, this direction we agree about. But your change is enforcing it the other way around (if barrier then no data), which wasn''t the case so far.>> Hence shouldn''t you clear the sector number only when >> req->nr_segments is zero? Or alternatively, shouldn''t > > We could do that too. > >> vbd_translate() simply not fail when req->nr_sects is zero? > > It does not fail when req->nr_sects is zero. It fails when it is -1. > >> >> Additionally, looking at the check in vbd_translate(), wouldn''t you >> think there ought to be overflow checking for the addition, too? > > Sure, could add that in. Albeit it seems incorrect to do it in that > function. It checks to see if the sector is correct, and -1 is definitly > wrong.Hmm, depends on your perspective - I''d say that any sector_number is valid when nr_sects is zero. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-May-17 15:57 UTC
Re: [Xen-devel] [GIT PULL] for-2.6.32/bug-fixes
> > No attaching of data to the barrier. > > Sure, this direction we agree about. But your change is enforcing > it the other way around (if barrier then no data), which wasn''t the > case so far.OK, even if the code that actually does the bio submission does not attach any data to the bio? The end result is the same - no data with barriers.> > >> Hence shouldn''t you clear the sector number only when > >> req->nr_segments is zero? Or alternatively, shouldn''t > > > > We could do that too. > > > >> vbd_translate() simply not fail when req->nr_sects is zero? > > > > It does not fail when req->nr_sects is zero. It fails when it is -1. > > > >> > >> Additionally, looking at the check in vbd_translate(), wouldn''t you > >> think there ought to be overflow checking for the addition, too? > > > > Sure, could add that in. Albeit it seems incorrect to do it in that > > function. It checks to see if the sector is correct, and -1 is definitly > > wrong. > > Hmm, depends on your perspective - I''d say that any sector_number > is valid when nr_sects is zero.I concur. The value that is passed by the frontend is not zero. It is -1. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 17.05.11 at 17:57, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> > No attaching of data to the barrier. >> >> Sure, this direction we agree about. But your change is enforcing >> it the other way around (if barrier then no data), which wasn''t the >> case so far. > > OK, even if the code that actually does the bio submission does > not attach any data to the bio? The end result is the same - no > data with barriers.My problem is that I can''t see where attaching data would be skipped. The only thing I see is the BUG_ON() you pointed at earlier, checking that if there is no data, then this must be a barrier request.>> >> Additionally, looking at the check in vbd_translate(), wouldn''t you >> >> think there ought to be overflow checking for the addition, too? >> > >> > Sure, could add that in. Albeit it seems incorrect to do it in that >> > function. It checks to see if the sector is correct, and -1 is definitly >> > wrong. >> >> Hmm, depends on your perspective - I''d say that any sector_number >> is valid when nr_sects is zero. > > I concur. The value that is passed by the frontend is not zero. It is -1.Oh, you say both sector_number and nr_sects are -1? Looking again... No, that can''t be the case, the value starts out at zero in dispatch_rw_block_io(). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-May-17 16:43 UTC
Re: [Xen-devel] [GIT PULL] for-2.6.32/bug-fixes
On Tue, May 17, 2011 at 05:24:43PM +0100, Jan Beulich wrote:> >>> On 17.05.11 at 17:57, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > >> > No attaching of data to the barrier. > >> > >> Sure, this direction we agree about. But your change is enforcing > >> it the other way around (if barrier then no data), which wasn''t the > >> case so far. > > > > OK, even if the code that actually does the bio submission does > > not attach any data to the bio? The end result is the same - no > > data with barriers. > > My problem is that I can''t see where attaching data would be > skipped. The only thing I see is the BUG_ON() you pointed atWell, req->ns_segments = 0, so nseg is zero, which means all of those for loops never get executed.> earlier, checking that if there is no data, then this must be a > barrier request. > > >> >> Additionally, looking at the check in vbd_translate(), wouldn''t you > >> >> think there ought to be overflow checking for the addition, too? > >> > > >> > Sure, could add that in. Albeit it seems incorrect to do it in that > >> > function. It checks to see if the sector is correct, and -1 is definitly > >> > wrong. > >> > >> Hmm, depends on your perspective - I''d say that any sector_number > >> is valid when nr_sects is zero. > > > > I concur. The value that is passed by the frontend is not zero. It is -1. > > Oh, you say both sector_number and nr_sects are -1? Looking > again... No, that can''t be the case, the value starts out at zero > in dispatch_rw_block_io().No, wait. Argh. The req->nr_sects is 0 and req->sector_number is -1. This is what I got before the fix: access denied: write of [18446744073709551615,18446744073709551615] on dev=ca0 And req->nr_segments is 0. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Konrad Rzeszutek Wilk 05/17/11 6:44 PM >>> >On Tue, May 17, 2011 at 05:24:43PM +0100, Jan Beulich wrote: >> >>> On 17.05.11 at 17:57, Konrad Rzeszutek Wilk wrote: >> >> > No attaching of data to the barrier. >> >> >> >> Sure, this direction we agree about. But your change is enforcing >> >> it the other way around (if barrier then no data), which wasn''t the >> >> case so far. >> > >> > OK, even if the code that actually does the bio submission does >> > not attach any data to the bio? The end result is the same - no >> > data with barriers. >> >> My problem is that I can''t see where attaching data would be >> skipped. The only thing I see is the BUG_ON() you pointed at > >Well, req->ns_segments = 0, so nseg is zero, which means all >of those for loops never get executed.This you say is the case for the request you saw the failure with, or *all* barrier requests? In the latter case, what do you conclude this from? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-May-18 13:24 UTC
Re: [Xen-devel] [GIT PULL] for-2.6.32/bug-fixes
> >Well, req->ns_segments = 0, so nseg is zero, which means all > >of those for loops never get executed. > > This you say is the case for the request you saw the failure with, or > *all* barrier requests? In the latter case, what do you conclude thisGood question. It was the first barrier request sent when guest tried to mount the filesystem. I will instrument the code to see what the other barriers contained when they were sent.> from?_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 18.05.11 at 15:24, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> >Well, req->ns_segments = 0, so nseg is zero, which means all >> >of those for loops never get executed. >> >> This you say is the case for the request you saw the failure with, or >> *all* barrier requests? In the latter case, what do you conclude this > > Good question. It was the first barrier request sent when guest tried to > mount the filesystem. I will instrument the code to see what the other > barriers contained when they were sent.That wouldn''t tell you anything if they''re all empty, as there''s nothing preventing other guests (including other guest OSes) to still send non-empty ones - after all the protocol allows for this. Jan>> from?_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-May-18 14:56 UTC
Re: [Xen-devel] [GIT PULL] for-2.6.32/bug-fixes
On Wed, May 18, 2011 at 03:31:35PM +0100, Jan Beulich wrote:> >>> On 18.05.11 at 15:24, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > >> >Well, req->ns_segments = 0, so nseg is zero, which means all > >> >of those for loops never get executed. > >> > >> This you say is the case for the request you saw the failure with, or > >> *all* barrier requests? In the latter case, what do you conclude this > > > > Good question. It was the first barrier request sent when guest tried to > > mount the filesystem. I will instrument the code to see what the other > > barriers contained when they were sent. > > That wouldn''t tell you anything if they''re all empty, as there''s nothing > preventing other guests (including other guest OSes) to still send > non-empty ones - after all the protocol allows for this.Aha! That is what you been trying to tell me. I will make a patch to make sure to not overwrite the req->sector_number blindly. What other guest OSes use barriers? I looked at Solaris (it uses ''feature-flush-cache''), NetBSD (''feature-flush-cache'') and Linux (''feature-barrier'' and now in 2.6.40 ''feature-flush-cache''). The GPLV Windows drivers have no barrier implementation - do you know if the Novell ones are using barriers? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 18.05.11 at 16:56, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Wed, May 18, 2011 at 03:31:35PM +0100, Jan Beulich wrote: >> >>> On 18.05.11 at 15:24, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> >> >Well, req->ns_segments = 0, so nseg is zero, which means all >> >> >of those for loops never get executed. >> >> >> >> This you say is the case for the request you saw the failure with, or >> >> *all* barrier requests? In the latter case, what do you conclude this >> > >> > Good question. It was the first barrier request sent when guest tried to >> > mount the filesystem. I will instrument the code to see what the other >> > barriers contained when they were sent. >> >> That wouldn''t tell you anything if they''re all empty, as there''s nothing >> preventing other guests (including other guest OSes) to still send >> non-empty ones - after all the protocol allows for this. > > Aha! That is what you been trying to tell me. I will make a patch to make > sure to not overwrite the req->sector_number blindly. What other guest OSesOr use the patch I proposed?> use barriers? I looked at Solaris (it uses ''feature-flush-cache''), NetBSD > (''feature-flush-cache'') and Linux (''feature-barrier'' and now in 2.6.40 > ''feature-flush-cache''). > > The GPLV Windows drivers have no barrier implementation - do you know if > the Novell ones are using barriers?No, I don''t. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Konrad Rzeszutek Wilk
2011-May-18 15:13 UTC
Re: [Xen-devel] [GIT PULL] for-2.6.32/bug-fixes
> >> That wouldn''t tell you anything if they''re all empty, as there''s nothing > >> preventing other guests (including other guest OSes) to still send > >> non-empty ones - after all the protocol allows for this. > > > > Aha! That is what you been trying to tell me. I will make a patch to make > > sure to not overwrite the req->sector_number blindly. What other guest OSes > > Or use the patch I proposed?Yes. Much superior to mine.> > > use barriers? I looked at Solaris (it uses ''feature-flush-cache''), NetBSD > > (''feature-flush-cache'') and Linux (''feature-barrier'' and now in 2.6.40 > > ''feature-flush-cache''). > > > > The GPLV Windows drivers have no barrier implementation - do you know if > > the Novell ones are using barriers? > > No, I don''t.Any idea who might know? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 18.05.11 at 17:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> >> That wouldn''t tell you anything if they''re all empty, as there''s nothing >> >> preventing other guests (including other guest OSes) to still send >> >> non-empty ones - after all the protocol allows for this. >> > >> > Aha! That is what you been trying to tell me. I will make a patch to make >> > sure to not overwrite the req->sector_number blindly. What other guest OSes >> >> Or use the patch I proposed? > > Yes. Much superior to mine. >> >> > use barriers? I looked at Solaris (it uses ''feature-flush-cache''), NetBSD >> > (''feature-flush-cache'') and Linux (''feature-barrier'' and now in 2.6.40 >> > ''feature-flush-cache''). >> > >> > The GPLV Windows drivers have no barrier implementation - do you know if >> > the Novell ones are using barriers? >> >> No, I don''t. > > Any idea who might know?Kirk should. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel