From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kiyoshi Ueda Subject: Re: [PATCH v2 1/2] dm: prevent table type changes after initial table load Date: Tue, 25 May 2010 20:16:22 +0900 Message-ID: <4BFBB186.8080207@ct.jp.nec.com> References: <1274744795-9825-1-git-send-email-snitzer@redhat.com> <1274744795-9825-2-git-send-email-snitzer@redhat.com> Reply-To: device-mapper development Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1274744795-9825-2-git-send-email-snitzer@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Mike Snitzer Cc: dm-devel@redhat.com, Alasdair Kergon List-Id: dm-devel.ids Hi Mike, On 05/25/2010 08:46 AM +0900, Mike Snitzer wrote: > Before table_load() stages an inactive table atomically check if its > type conflicts with a previously established device type (md->type). > > Introduce 'type_lock' in mapped_device structure and use it to protect > md->type access. table_load() sets md->type without concern for: > - another table_load() racing to set conflicting md->type. > - do_resume() making a conflicting table live. > > Allowed transitions: > UNKNOWN_MD_TYPE => BIO_BASED_MD_TYPE > UNKNOWN_MD_TYPE => REQUEST_BASED_MD_TYPE > > Once a table load occurs the DM device's type is completely immutable. > > This prevents a table reload from switching the inactive table directly > to a conflicting type (even if the table is explicitly cleared before > load). > > Signed-off-by: Mike Snitzer I basically agree with this design. But I have some comments. Please see below. By the way, I can't make enough time to review until tomorrow. So the following comments may not be all. Sorry for the inconvenience. > Index: linux-2.6/drivers/md/dm-ioctl.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-ioctl.c > +++ linux-2.6/drivers/md/dm-ioctl.c > @@ -1176,12 +1176,39 @@ static int table_load(struct dm_ioctl *p > goto out; > } > > + /* > + * Protect md->type against concurrent table loads. > + * Locking strategy: > + * + Leverage fact that md's type cannot change after initial table load. > + * - Only protect type in table_load() -- not in do_resume(). > + * > + * + Protect type while working to stage an inactive table: > + * - check if table's type conflicts with md->type > + * (holding: md->type_lock) > + * - stage inactive table (hc->new_map) > + * (holding: md->type_lock + _hash_lock) > + */ I think you don't need to hold md->type_lock while you set the table to hc->new_map; If 2 tables of the same type are racing here, we can have either table. If they are different type, the former one will win and the latter one will be rejected. > + dm_lock_md_type(md); > + > + if (dm_unknown_md_type(md)) { > + /* initial table load, set md's type based on table's type */ > + dm_set_md_type(md, t); > + } else if (!dm_md_type_matches_table(md, t)) { > + DMWARN("can't change device type after initial table load."); > + dm_table_destroy(t); > + dm_unlock_md_type(md); > + r = -EINVAL; > + goto out; > + } So you should be able to unlock md->type_lock here. > Index: linux-2.6/drivers/md/dm.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm.c > +++ linux-2.6/drivers/md/dm.c > @@ -111,6 +111,15 @@ EXPORT_SYMBOL_GPL(dm_get_rq_mapinfo); > #define DMF_QUEUE_IO_TO_THREAD 6 > > /* > + * Type for md->type field. > + */ > +enum mapped_device_type { > + UNKNOWN_MD_TYPE, > + BIO_BASED_MD_TYPE, > + REQUEST_BASED_MD_TYPE, > +}; You can use the defined types in drivers/md/dm.h below instead; /* * Type of table and mapped_device's mempool */ #define DM_TYPE_NONE 0 #define DM_TYPE_BIO_BASED 1 #define DM_TYPE_REQUEST_BASED 2 Using them should make type matching easier than making another definition. E.g. see below; > +bool dm_md_type_matches_table(struct mapped_device *md, struct dm_table* t) > +{ > + if (dm_request_based_md_type(md)) > + return dm_table_request_based(t); > + else if (dm_bio_based_md_type(md)) > + return !dm_table_request_based(t); > + > + return 0; > +} You can match type using "md->type == dm_table_get_type(t)" if you use the already defined types above. Thanks, Kiyoshi Ueda