Xen devel - Apr 2010 - [PATCH 00 of 10] blkfront pvops updates

Hi.

Here are couple of bugfixes for blkfront.

 1. Bdev removal was slightly buggy. [Is nobody out there using
    block-detach??] Also it seems like we''be been leaking the gendisk
    structs for a long time.

 2. Fixes most common occasions where disk open/close races against
    backend state switches. This one didn''t come for free:
	* xenbus_switch_state growing transactions again.
	* New mutex in blkfront_info
	* Bdev .open/.close rewritten accordingly.

Cheers,
Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

According to the comments, this was how it''s been done years ago, but
apparently took an xbt pointer from elsewhere back then. The code was
removed because of consistency issues: cancellation wont''t roll back
the saved xbdev->state.

Still, unsolicited writes to the state field remain an issue,
especially if device shutdown takes thread synchronization, and subtle
races cause accidental recreation of the device node.

Fixed by reintroducing the transaction. An internal one is sufficient,
so the xbdev->state value remains consistent.

Also fixes the original hack to prevent infinite recursion. Instead of
bailing out on the first attempt to switch to Closing, checks call
depth now.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The call to del_gendisk follows an non-refcounted gd->queue
pointer. We release the last ref in blk_cleanup_queue. Fixed by
reordering releases accordingly.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

* Current blkfront_closing is rather a xlvbd_release_gendisk.
   Renamed in preparation of later patches (need the name again).

 * Removed the misleading comment -- this only applied to the backend
   switch handler, and the queue is already flushed btw.

 * Break out the xenbus call, callers know better when to switch
   frontend state.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

The bdev .open/.release fops race against backend switches to Closing,
handled by the XenBus thread.

The original code attempted to serialize block device holders and
xenbus only via bd_mutex. This is insufficient, the info->bd pointer
may already be stale (or null) while xenbus tries to bump up the
refcount. Worst case scenario is a null ptr deref.

Protect blkfront_info with a dedicated mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

We need not mind if users grab a late handle on a closing disk. We
probably even should not. But we have to make sure it''s not a dead
one already

Let the bdev deal with a gendisk deleted under its feet. Takes the
info mutex to decide a race against backend closing.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

We cannot read backend state within bdev operations, because it risks
grabbing the state change before xenbus gets to do it.

Fixed by tracking deferral with a frontend switch to Closing. State
exposure isn''t strictly necessary, but the backends won''t
mind.

For a ''clean'' deferral this seems actually a more decent
protocol than
raising errors.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Same approach as blkfront_closing:
 * Grab the bdev safely, holding the info mutex.
 * Zap xbdev safely, holding the info mutex.
 * Try bdev removal safely, holding bd_mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

This is just bd_openers, protected by the bd_mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/28/2010 07:19 PM, Daniel Stodden wrote:
> Hi.
>
> Here are couple of bugfixes for blkfront.
>
>  1. Bdev removal was slightly buggy. [Is nobody out there using
>     block-detach??] Also it seems like we''be been leaking the
gendisk
>     structs for a long time.
>
>  2. Fixes most common occasions where disk open/close races against
>     backend state switches. This one didn''t come for free:
> 	* xenbus_switch_state growing transactions again.
> 	* New mutex in blkfront_info
> 	* Bdev .open/.close rewritten accordingly.
>   
Thanks for this.  However, when I went to apply it, it looks like I''ve
made a mess of the various blkfront branches. 

xen/frontend has some changes from Jan and Ky which are merged into
xen/next and the various stable branches.

I also have xen/blkfront, which has your last set of changes, which I
seem to have forgotten to merge into anything else, and now conflicts
non-trivially with xen/blkfront.  I''d like to add this new set of
patches into xen/blkfront, but it doesn''t apply cleanly.

Could you look at this and advise me what I should do?

Thanks,
    J
> Cheers,
> Daniel
>
>   

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-04-29 at 15:50 -0400, Jeremy Fitzhardinge
wrote:
> On 04/28/2010 07:19 PM, Daniel Stodden wrote:
> > Hi.
> >
> > Here are couple of bugfixes for blkfront.
> >
> >  1. Bdev removal was slightly buggy. [Is nobody out there using
> >     block-detach??] Also it seems like we''be been leaking the
gendisk
> >     structs for a long time.
> >
> >  2. Fixes most common occasions where disk open/close races against
> >     backend state switches. This one didn''t come for free:
> > 	* xenbus_switch_state growing transactions again.
> > 	* New mutex in blkfront_info
> > 	* Bdev .open/.close rewritten accordingly.
> >   
> 
> Thanks for this.  However, when I went to apply it, it looks like
I''ve
> made a mess of the various blkfront branches. 
> 
> xen/frontend has some changes from Jan and Ky which are merged into
> xen/next and the various stable branches.
> 
> I also have xen/blkfront, which has your last set of changes, which I
> seem to have forgotten to merge into anything else, and now conflicts
> non-trivially with xen/blkfront.  I''d like to add this new set of
> patches into xen/blkfront, but it doesn''t apply cleanly.
> 
> Could you look at this and advise me what I should do?
Oh, NP. I''ll try and see if I can gather those and what it takes to
merge.

Daniel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-04-29 at 16:11 -0400, Daniel Stodden wrote:
> On Thu, 2010-04-29 at 15:50 -0400, Jeremy Fitzhardinge wrote:
> > On 04/28/2010 07:19 PM, Daniel Stodden wrote:
> > > Hi.
> > >
> > > Here are couple of bugfixes for blkfront.
> > >
> > >  1. Bdev removal was slightly buggy. [Is nobody out there using
> > >     block-detach??] Also it seems like we''be been
leaking the gendisk
> > >     structs for a long time.
> > >
> > >  2. Fixes most common occasions where disk open/close races
against
> > >     backend state switches. This one didn''t come for
free:
> > > 	* xenbus_switch_state growing transactions again.
> > > 	* New mutex in blkfront_info
> > > 	* Bdev .open/.close rewritten accordingly.
> > >   
> > 
> > Thanks for this.  However, when I went to apply it, it looks like
I''ve
> > made a mess of the various blkfront branches. 
> > 
> > xen/frontend has some changes from Jan and Ky which are merged into
> > xen/next and the various stable branches.
> > 
> > I also have xen/blkfront, which has your last set of changes, which I
> > seem to have forgotten to merge into anything else, and now conflicts
> > non-trivially with xen/blkfront.  I''d like to add this new
set of
> > patches into xen/blkfront, but it doesn''t apply cleanly.
> > 
> > Could you look at this and advise me what I should do?
> 
> Oh, NP. I''ll try and see if I can gather those and what it takes
to
> merge.
Ah, that''s where the previous stuff ended up. I thought those probably
just got dropped.

Let''s just scratch those. It''s been months, so if they
didn''t make it
back then, they won''t need to complicate stuff now either.

So I should probably merge with xen/frontend. If those matter, they must
be on some branch which matters. Which one is it? I thought the one
which matters is xen/master? Is it xen/next?

Daniel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/29/2010 01:58 PM, Daniel Stodden wrote:
> Ah, that''s where the previous stuff ended up. I thought those
probably
> just got dropped.
>   
I thought the changes in these patches looked somewhat familiar.
> Let''s just scratch those. It''s been months, so if they
didn''t make it
> back then, they won''t need to complicate stuff now either.
>
> So I should probably merge with xen/frontend. If those matter, they must
> be on some branch which matters. Which one is it? I thought the one
> which matters is xen/master? Is it xen/next?
>   
The flow of changes is (ideally):

    <topic branch> -> xen/next -> xen/stable*

where "->" is merged into.

I''m going to clean things up a bit by re-creating xen/blkfront with the
changes from xen/frontend.  If you rebase your patches onto xen/frontend
then I should be able to easily incorporate them into that cleanup.

Sorry for the mixup.

Thanks,
    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-04-29 at 17:10 -0400, Jeremy Fitzhardinge
wrote:
> On 04/29/2010 01:58 PM, Daniel Stodden wrote:
> > Ah, that''s where the previous stuff ended up. I thought those
probably
> > just got dropped.
> >   
> 
> I thought the changes in these patches looked somewhat familiar.
> 
> > Let''s just scratch those. It''s been months, so if
they didn''t make it
> > back then, they won''t need to complicate stuff now either.
> >
> > So I should probably merge with xen/frontend. If those matter, they
must
> > be on some branch which matters. Which one is it? I thought the one
> > which matters is xen/master? Is it xen/next?
> >   
> 
> The flow of changes is (ideally):
> 
>     <topic branch> -> xen/next -> xen/stable*
> 
> where "->" is merged into.
So I''m probably building the wrong tree by default.

I used to make linux-2.6-pvops when starting to work.
Which checks out xen/master.

How does this relate?

Daniel
> I''m going to clean things up a bit by re-creating xen/blkfront
with the
> changes from xen/frontend.  If you rebase your patches onto xen/frontend
> then I should be able to easily incorporate them into that cleanup.
> 
> Sorry for the mixup.
> 
> Thanks,
>     J


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/29/2010 02:28 PM, Daniel Stodden wrote:
> So I''m probably building the wrong tree by default.
>
> I used to make linux-2.6-pvops when starting to work.
> Which checks out xen/master.
>
> How does this relate?
>   
xen/master is an alias for xen/stable-2.6.31.x at the moment.  I just
submitted a patch to make this switchover explicit, and will move it to
2.6.32 soon.

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-04-29 at 17:36 -0400, Jeremy Fitzhardinge
wrote:
> On 04/29/2010 02:28 PM, Daniel Stodden wrote:
> > So I''m probably building the wrong tree by default.
> >
> > I used to make linux-2.6-pvops when starting to work.
> > Which checks out xen/master.
> >
> > How does this relate?
> >   
> 
> xen/master is an alias for xen/stable-2.6.31.x at the moment.  I just
> submitted a patch to make this switchover explicit, and will move it to
> 2.6.32 soon.
Okay, so xen/master rather defaults to some stable tree.

If xen/frontend is the topic branch, people ideally prepare patches per
topic, and trunk is rather xen/next, then the thing I still don''t
follow
is than xen/frontend hasn''t been merged since

* commit 8800a1de3df00fa994f72ad0d7c1eda9b5a0b514
| Author: Paolo Bonzini <pbonzini@redhat.com>
| Date:   Wed Jul 8 12:27:37 2009 +0200

Why would I want to build a kernel derived from last July''s state? Or,
put differently: Is it okay to merge these topic branches before
anything else?

Daniel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2010-04-29 at 16:11 -0400, Daniel Stodden wrote:
> On Thu, 2010-04-29 at 15:50 -0400, Jeremy Fitzhardinge wrote:
> > On 04/28/2010 07:19 PM, Daniel Stodden wrote:
> > > Hi.
> > >
> > > Here are couple of bugfixes for blkfront.
> > >
> > >  1. Bdev removal was slightly buggy. [Is nobody out there using
> > >     block-detach??] Also it seems like we''be been
leaking the gendisk
> > >     structs for a long time.
> > >
> > >  2. Fixes most common occasions where disk open/close races
against
> > >     backend state switches. This one didn''t come for
free:
> > > 	* xenbus_switch_state growing transactions again.
> > > 	* New mutex in blkfront_info
> > > 	* Bdev .open/.close rewritten accordingly.
> > >   
> > 
> > Thanks for this.  However, when I went to apply it, it looks like
I''ve
> > made a mess of the various blkfront branches. 
> > 
> > xen/frontend has some changes from Jan and Ky which are merged into
> > xen/next and the various stable branches.
> > 
> > I also have xen/blkfront, which has your last set of changes, which I
> > seem to have forgotten to merge into anything else, and now conflicts
> > non-trivially with xen/blkfront.  I''d like to add this new
set of
> > patches into xen/blkfront, but it doesn''t apply cleanly.
> > 
> > Could you look at this and advise me what I should do?
> 
> Oh, NP. I''ll try and see if I can gather those and what it takes
to
> merge.
I moved on to xen/next. That still seems to apply okay.

But as usual when looking at that kind of thing a day later, I promptly
found 2 more loopholes I didn''t see before. %}

These should be closed now too, but I''ll rather rerun this stuff
tomorrow again before resubmitting.

Thanks,
Daniel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/29/2010 04:05 PM, Daniel Stodden wrote:
> On Thu, 2010-04-29 at 17:36 -0400, Jeremy Fitzhardinge wrote:
>   
>> On 04/29/2010 02:28 PM, Daniel Stodden wrote:
>>     
>>> So I''m probably building the wrong tree by default.
>>>
>>> I used to make linux-2.6-pvops when starting to work.
>>> Which checks out xen/master.
>>>
>>> How does this relate?
>>>   
>>>       
>> xen/master is an alias for xen/stable-2.6.31.x at the moment.  I just
>> submitted a patch to make this switchover explicit, and will move it to
>> 2.6.32 soon.
>>     
> Okay, so xen/master rather defaults to some stable tree.
>
> If xen/frontend is the topic branch, people ideally prepare patches per
> topic, and trunk is rather xen/next, then the thing I still don''t
follow
> is than xen/frontend hasn''t been merged since
>
> * commit 8800a1de3df00fa994f72ad0d7c1eda9b5a0b514
> | Author: Paolo Bonzini <pbonzini@redhat.com>
> | Date:   Wed Jul 8 12:27:37 2009 +0200
>
> Why would I want to build a kernel derived from last July''s state?
Or,
> put differently: Is it okay to merge these topic branches before
> anything else?
>   
My usual workflow is:

   1. Develop some patches on xen/next, until they work
   2. Rebase those patches onto a topic branch
   3. Merge that topic branch onto xen/next

This is necessary because an individual topic branch isn''t necessarily
compilable into a working kernel (though they should always compile).

The topic branches are often based on very old kernel versions, and I
avoid rebasing or merging new kernels into them for as long as
possible.  This is so that they will share a common base version with
any other branch I want to merge them into (including mainline Linux),
so that 3-way merging is most likely to work.

It gets awkward if xen/next has other changes which conflict with the
topic branch either because some other piece of necessary infrastructure
is missing, or just plain merge conflicts.  If they''re minor then I
resolve them in the merge to xen/next (git rerere is your friend for
remembering these kinds of conflict resolutions).   But if there''s some
major subsystem change, then I''ll update the topic branch by merging in
an appropriate branch to update the branch (ideally always from the
mainline Linux branch, or from another Xen topic branch).

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 04/29/2010 04:05 PM, Daniel Stodden wrote:
> On Thu, 2010-04-29 at 17:36 -0400, Jeremy Fitzhardinge wrote:
>   
>> On 04/29/2010 02:28 PM, Daniel Stodden wrote:
>>     
>>> So I''m probably building the wrong tree by default.
>>>
>>> I used to make linux-2.6-pvops when starting to work.
>>> Which checks out xen/master.
>>>
>>> How does this relate?
>>>   
>>>       
>> xen/master is an alias for xen/stable-2.6.31.x at the moment.  I just
>> submitted a patch to make this switchover explicit, and will move it to
>> 2.6.32 soon.
>>     
> Okay, so xen/master rather defaults to some stable tree.
>
> If xen/frontend is the topic branch, people ideally prepare patches per
> topic, and trunk is rather xen/next, then the thing I still don''t
follow
> is than xen/frontend hasn''t been merged since
>
> * commit 8800a1de3df00fa994f72ad0d7c1eda9b5a0b514
> | Author: Paolo Bonzini <pbonzini@redhat.com>
> | Date:   Wed Jul 8 12:27:37 2009 +0200
>   
BTW, it looks like you haven''t updated your tree in a while.  That
branch has a few more changes on it since that one:

ad9ecc8ebfae8cab30ea2467b987f9b18b3172cc xen/blkfront: revalidate after setting
capacity
7a634554e8ba2be858448bcf9617527c34ce326b xen/blkfront: avoid compiler warning
from missing cases
065157270688777b934770fa4dcfcd68800560c3 xen/front: Propagate changed size of
VBDs
1ebf91c24ab17ee62afeb1640278dd02da781fd7 blkfront: don''t access freed
struct xenbus_device
eba64a07039d46222ca9d157c8c0549f4177f53d xen: fbdev frontend needs xenbus
frontend
eebc80638fe365a0386ed5ee57c7f0c6d5e56fb1 blkfront: fixes for ''xm
block-detach ... --force''
71133087313f15db44ffb6ea802e5bdb2479a600 xen: use less generic names in blkfront
driver.
9c9c87f53f87d0368ef04207cce4c92884f4ae3d xen: use less generic names in netfront
driver.
11b84f69d36b518ff862a1bcd61ace7f8dac76b5 xen: wait up to 5 minutes for device
connetion
ed9ef1e71122628b50b0a526607509511b0d9135 xen: improvement to wait_for_devices()

	J


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2010-04-30 at 14:13 -0400, Jeremy Fitzhardinge
wrote:
> On 04/29/2010 04:05 PM, Daniel Stodden wrote:
> > On Thu, 2010-04-29 at 17:36 -0400, Jeremy Fitzhardinge wrote:
> >   
> >> On 04/29/2010 02:28 PM, Daniel Stodden wrote:
> >>     
> >>> So I''m probably building the wrong tree by default.
> >>>
> >>> I used to make linux-2.6-pvops when starting to work.
> >>> Which checks out xen/master.
> >>>
> >>> How does this relate?
> >>>   
> >>>       
> >> xen/master is an alias for xen/stable-2.6.31.x at the moment.  I
just
> >> submitted a patch to make this switchover explicit, and will move
it to
> >> 2.6.32 soon.
> >>     
> > Okay, so xen/master rather defaults to some stable tree.
> >
> > If xen/frontend is the topic branch, people ideally prepare patches
per
> > topic, and trunk is rather xen/next, then the thing I still
don''t follow
> > is than xen/frontend hasn''t been merged since
> >
> > * commit 8800a1de3df00fa994f72ad0d7c1eda9b5a0b514
> > | Author: Paolo Bonzini <pbonzini@redhat.com>
> > | Date:   Wed Jul 8 12:27:37 2009 +0200
> >   
> 
> BTW, it looks like you haven''t updated your tree in a while.  That
> branch has a few more changes on it since that one:
> 
> ad9ecc8ebfae8cab30ea2467b987f9b18b3172cc xen/blkfront: revalidate after
setting capacity
> 7a634554e8ba2be858448bcf9617527c34ce326b xen/blkfront: avoid compiler
warning from missing cases
> 065157270688777b934770fa4dcfcd68800560c3 xen/front: Propagate changed size
of VBDs
> 1ebf91c24ab17ee62afeb1640278dd02da781fd7 blkfront: don''t access
freed struct xenbus_device
> eba64a07039d46222ca9d157c8c0549f4177f53d xen: fbdev frontend needs xenbus
frontend
> eebc80638fe365a0386ed5ee57c7f0c6d5e56fb1 blkfront: fixes for ''xm
block-detach ... --force''
> 71133087313f15db44ffb6ea802e5bdb2479a600 xen: use less generic names in
blkfront driver.
> 9c9c87f53f87d0368ef04207cce4c92884f4ae3d xen: use less generic names in
netfront driver.
> 11b84f69d36b518ff862a1bcd61ace7f8dac76b5 xen: wait up to 5 minutes for
device connetion
> ed9ef1e71122628b50b0a526607509511b0d9135 xen: improvement to
wait_for_devices()
No, I certainly got those too. I was just surprised about where that
branch was rooted. Okay, so sort of a revision- vs. bare patch-
management thing then. Thanks for the clarification. 143 branches just
started to make sort of sense. :o)

Daniel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel