FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 01-10-2012, 09:31 AM
Joe Thornber
 
Default dm thin metadata: clarify __open_device functionality and fix callers

On Mon, Jan 09, 2012 at 03:33:08PM -0500, Mike Snitzer wrote:
> The __open_device() error path of __create_thin() and __create_snap()
> will call __close_device() even if td was not initialized by
> __open_device().
>
> Clearly define what callers should expect from __open_device()'s
> return.
>
> Also, remove redundant 'td->changed = 1' in __create_thin() --
> __open_device() with create=1 will have set td->changed on successful
> return.

Hi Mike,

This looks like a code review patch, and as such I ACK it.

I don't think it changes any behaviour though. Both create_thin and
create_snapshot hold a mutex when they run, and the first thing they
do is check for the existence of the device.

r = dm_btree_lookup(&pmd->details_info, pmd->details_root,
&key, &details_le);
if (!r)
return -EEXIST;

I don't think there's a race on creation?

We need to focus on reproducing Kabi's issue.

- Joe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 01-10-2012, 01:22 PM
Mike Snitzer
 
Default dm thin metadata: clarify __open_device functionality and fix callers

On Tue, Jan 10 2012 at 5:31am -0500,
Joe Thornber <thornber@redhat.com> wrote:

> On Mon, Jan 09, 2012 at 03:33:08PM -0500, Mike Snitzer wrote:
> > The __open_device() error path of __create_thin() and __create_snap()
> > will call __close_device() even if td was not initialized by
> > __open_device().
> >
> > Clearly define what callers should expect from __open_device()'s
> > return.
> >
> > Also, remove redundant 'td->changed = 1' in __create_thin() --
> > __open_device() with create=1 will have set td->changed on successful
> > return.
>
> Hi Mike,
>
> This looks like a code review patch, and as such I ACK it.

Yes, this was something that Alasdair came across when he inspected the
error path of __create_thin(). He was looking at this because Kabi was
having issues with opencount on a DM device (device held open when he
wasn't aware of any openers).

> I don't think it changes any behaviour though. Both create_thin and
> create_snapshot hold a mutex when they run, and the first thing they
> do is check for the existence of the device.
>
> r = dm_btree_lookup(&pmd->details_info, pmd->details_root,
> &key, &details_le);
> if (!r)
> return -EEXIST;
>
> I don't think there's a race on creation?

Right, doesn't change any behavior -- other than fix the error path to
not decrement some random memory (uninitialized @td) in
__close_device().

(and no I wasn't suggesting there was some race on creation).

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 08:33 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org