Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 00 of 10] blkfront pvops updates, v2
Updates since the last time: * Make sure everything applies against xen/frontend. That fortunately made no difference. * Two more loopholes: - The path freeing info in xenbus_remove took a more carefully sync with an unfortunate bdops->release. This is because the locking can''t just nest. Fixed by resyncing the info pointer through disk->private_data. - Similar effect during bdev open. This also needs to anticipate the not-so-unlikely case where not only the gendisk but the entire info struct was deleted just before entry. Again, syncing on disk->private_data once holding bd_mutex does the trick. I chose to just merge those in. * To be honest, that dev_warn the last patch added was broken -- doh. * Found the switch [again] for HG to strip those headers -- cheers. Feedback certainly welcome. I am aware this thing looks manic, but at it stands, we recently happend to come across exactly that family of races in XCP again, so I really want these paths fixed. Thanks, Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 01 of 10] xenbus: Make xenbus_switch_state transactional (again)
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
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 02 of 10] blkfront: Fix backtrace in del_gendisk
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
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 03 of 10] blkfront: Fix gendisk leak
Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 04 of 10] blkfront: Clean up vbd release
* 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
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 05 of 10] blkfront: Lock blkfront_info when closing
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. 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
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 06 of 10] blkfront: Fix blkfront backend switch race (bdev open)
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
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 07 of 10] blkfront: Fix blkfront backend switch race (bdev release)
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
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 08 of 10] blkfront: Lock blockfront_info during xbdev removal
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
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 09 of 10] blkfront: Remove obsolete info->users
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
Daniel Stodden
2010-Apr-30 22:01 UTC
[Xen-devel] [PATCH 10 of 10] blkfront: Klog the unclean release path
Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Apr-30 22:16 UTC
[Xen-devel] Re: [PATCH 00 of 10] blkfront pvops updates, v2
On 04/30/2010 03:01 PM, Daniel Stodden wrote:> * Found the switch [again] for HG to strip those headers -- cheers. >The ideal for me is if you have a git tree I can pull from, but failing that, the most useful format for mailed patches is something that "git am" can accept. It looks like it will take the attachments OK, but it cats the mail body onto the attachment, so it ends up with the commit comment twice. If you have a non-mangling mailer, it''s probably easiest if you just include the full commit comment+patch in the mail body (it''s easier to comment on the patches in mail that way too).> Oh beautiful. This apparently strips the first comment line together > with the patch header. Looks buggy. > > F*! > > Sorry. Fix/resend? >Did it lose something other than the subject? git am picked it up from the email Subject: line, so it didn''t matter it was missing from the patch comment itself. J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Daniel Stodden
2010-Apr-30 22:31 UTC
[Xen-devel] Re: [PATCH 00 of 10] blkfront pvops updates, v2
On Fri, 2010-04-30 at 18:16 -0400, Jeremy Fitzhardinge wrote:> On 04/30/2010 03:01 PM, Daniel Stodden wrote: > > * Found the switch [again] for HG to strip those headers -- cheers. > > > > The ideal for me is if you have a git tree I can pull from,There''s still hope to get one. Didn''t happen yet. Ian?> but failing > that, the most useful format for mailed patches is something that "git > am" can accept. It looks like it will take the attachments OK, but it > cats the mail body onto the attachment, so it ends up with the commit > comment twice. If you have a non-mangling mailer, it''s probably easiest > if you just include the full commit comment+patch in the mail body (it''s > easier to comment on the patches in mail that way too).Okay, whatever works.> > Oh beautiful. This apparently strips the first comment line together > > with the patch header. Looks buggy. > > > > F*! > > > > Sorry. Fix/resend? > > > > Did it lose something other than the subject?No.> git am picked it up from > the email Subject: line, so it didn''t matter it was missing from the > patch comment itself.Good. Thanks, Daniel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel