Re: [dm-devel] [PATCH 21/24] dm-dirty-log: allow log size to be different from target size.

[Neil Brown <neilb@suse.de>]

[Insert obligatory grumble about Reply-to headers inserted by
 mail-list software.  Adding linux-raid back to the cc list...]

On Wed, 02 Jun 2010 16:57:21 +0200
Heinz Mauelshagen <heinzm@redhat.com> wrote:

> Neil,
> 
> I had a first review through your patch series, which look mostly good
> to me.
> 
> I've got 2 points so far before I go for tests and further review:
> 
> o there's actually no need to change the dm-dirty-log interface in an
>   incompatible way to allow for what's needed (see patch attached on top
>   of your series) which we can't do in RHEL/SUSE/... anyway.

Yes I know.  I saw that work-around in your original patch.
However this patch set isn't aimed at RHEL or SLES, it is aimed upstream.
And when we do things upstream we do them *right*.
If we then want to backport them to RHEL/SLES which require ABI
compatibility, then little hacks like the one you show (i.e. lying about the
size of the target when creating the log) may be perfectly appropriate.

> 
>   Notwithstanding, we need a discussion on dm-devel to justify if we
>   should change the API upstream in order to avoid such workaround as in
>   my attached patched.

Certainly it is appropriate to discuss this API change - that is why I
highlighted it in my introduction to the patch set.  Do you have an opinion
on it?


> 
> o any reason you limit the dm-dirty-log type to 'core' ?

That is only in the earlier part of the patch set.  md/raid5 can work without
a dirty log at all.  When it does, it's behaviour is completely analogous to
working with a 'core' dirty log.  So to get the raid5 part working without
needing to worry about the dirty-log stuff, I told md/raid5 not to use a
dirty log, and pretended it was using a 'core' log.

Then in later patches after I had enabled md/bitmap to work with dm-log, I
switched dm-raid45 over to use a real dm-log and removed the restriction to
only using a core log.
So this is really just an artefact of the order in which I developed the
code.  I could go back and re-arrange it so the dm-log integration comes
first, then I wouldn't need this intermediate stage which only supports
'core'.

Thanks,
NeilBrown


> 
> Heinz
> 
> On Tue, 2010-06-01 at 19:56 +1000, NeilBrown wrote:
> > With RAID1, the dirty log covers the size of the target - the number
> > of bits is the target size divided by the region size.
> > 
> > For RAID5 and similar the dirty log needs to cover the parity blocks,
> > so it is best to base the dirty log size on the size of the component
> > devices rather than the size of the array.
> > 
> > So when creating a dirty log, allow the log_size to be specified
> > separately from the target, and in raid1, set it to the target length.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  drivers/md/dm-log-userspace-base.c |   11 +++++++----
> >  drivers/md/dm-log.c                |   18 +++++++++++-------
> >  drivers/md/dm-raid1.c              |    4 ++--
> >  include/linux/dm-dirty-log.h       |    3 ++-
> >  4 files changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/md/dm-log-userspace-base.c b/drivers/md/dm-log-userspace-base.c
> > index 1ed0094..935a49b 100644
> > --- a/drivers/md/dm-log-userspace-base.c
> > +++ b/drivers/md/dm-log-userspace-base.c
> > @@ -94,7 +94,7 @@ retry:
> <SNIP>
> 
> 
>  drivers/md/dm-log.c          |   18 +++++++-----------
>  drivers/md/dm-raid1.c        |    4 ++--
>  drivers/md/dm-raid456.c      |    8 +++++---
>  include/linux/dm-dirty-log.h |    3 +--
>  4 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/md/dm-log.c b/drivers/md/dm-log.c
> index a232c14..5a08be0 100644
> --- a/drivers/md/dm-log.c
> +++ b/drivers/md/dm-log.c
> @@ -146,7 +146,6 @@ EXPORT_SYMBOL(dm_dirty_log_type_unregister);
>  
>  struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
>  			struct dm_target *ti,
> -			sector_t log_size,
>  			int (*flush_callback_fn)(struct dm_target *ti),
>  			unsigned int argc, char **argv)
>  {
> @@ -165,7 +164,7 @@ struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
>  
>  	log->flush_callback_fn = flush_callback_fn;
>  	log->type = type;
> -	if (type->ctr(log, ti, log_size, argc, argv)) {
> +	if (type->ctr(log, ti, argc, argv)) {
>  		kfree(log);
>  		put_type(type);
>  		return NULL;
> @@ -336,9 +335,9 @@ static int read_header(struct log_c *log)
>  	return 0;
>  }
>  
> -static int _check_region_size(sector_t log_size, uint32_t region_size)
> +static int _check_region_size(struct dm_target *ti, uint32_t region_size)
>  {
> -	if (region_size < 2 || region_size > log_size)
> +	if (region_size < 2 || region_size > ti->len)
>  		return 0;
>  
>  	if (!is_power_of_2(region_size))
> @@ -354,7 +353,6 @@ static int _check_region_size(sector_t log_size, uint32_t region_size)
>   *--------------------------------------------------------------*/
>  #define BYTE_SHIFT 3
>  static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
> -			      sector_t log_size,
>  			      unsigned int argc, char **argv,
>  			      struct dm_dev *dev)
>  {
> @@ -384,12 +382,12 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
>  	}
>  
>  	if (sscanf(argv[0], "%u", &region_size) != 1 ||
> -	    !_check_region_size(log_size, region_size)) {
> +	    !_check_region_size(ti, region_size)) {
>  		DMWARN("invalid region size %s", argv[0]);
>  		return -EINVAL;
>  	}
>  
> -	region_count = dm_sector_div_up(log_size, region_size);
> +	region_count = dm_sector_div_up(ti->len, region_size);
>  
>  	lc = kmalloc(sizeof(*lc), GFP_KERNEL);
>  	if (!lc) {
> @@ -509,10 +507,9 @@ static int create_log_context(struct dm_dirty_log *log, struct dm_target *ti,
>  }
>  
>  static int core_ctr(struct dm_dirty_log *log, struct dm_target *ti,
> -		    sector_t log_size,
>  		    unsigned int argc, char **argv)
>  {
> -	return create_log_context(log, ti, log_size, argc, argv, NULL);
> +	return create_log_context(log, ti, argc, argv, NULL);
>  }
>  
>  static void destroy_log_context(struct log_c *lc)
> @@ -536,7 +533,6 @@ static void core_dtr(struct dm_dirty_log *log)
>   * argv contains log_device region_size followed optionally by [no]sync
>   *--------------------------------------------------------------*/
>  static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
> -		    sector_t log_size,
>  		    unsigned int argc, char **argv)
>  {
>  	int r;
> @@ -551,7 +547,7 @@ static int disk_ctr(struct dm_dirty_log *log, struct dm_target *ti,
>  	if (r)
>  		return r;
>  
> -	r = create_log_context(log, ti, log_size, argc - 1, argv + 1, dev);
> +	r = create_log_context(log, ti, argc - 1, argv + 1, dev);
>  	if (r) {
>  		dm_put_device(ti, dev);
>  		return r;
> diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
> index ea732fc..ddda531 100644
> --- a/drivers/md/dm-raid1.c
> +++ b/drivers/md/dm-raid1.c
> @@ -968,8 +968,8 @@ static struct dm_dirty_log *create_dirty_log(struct dm_target *ti,
>  		return NULL;
>  	}
>  
> -	dl = dm_dirty_log_create(argv[0], ti, ti->len, mirror_flush,
> -				 param_count, argv + 2);
> +	dl = dm_dirty_log_create(argv[0], ti, mirror_flush, param_count,
> +				 argv + 2);
>  	if (!dl) {
>  		ti->error = "Error creating mirror dirty log";
>  		return NULL;
> diff --git a/drivers/md/dm-raid456.c b/drivers/md/dm-raid456.c
> index 3dcbc4a..33d2be8 100644
> --- a/drivers/md/dm-raid456.c
> +++ b/drivers/md/dm-raid456.c
> @@ -192,7 +192,7 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
>  	int recovery = 1;
>  	long raid_devs;
>  	long rebuildA, rebuildB;
> -	sector_t sectors_per_dev, chunks;
> +	sector_t sectors_per_dev, chunks, ti_len_sav;
>  	struct raid_set *rs = NULL;
>  	int in_sync, i;
>  	struct dm_dirty_log *log = NULL;
> @@ -281,8 +281,10 @@ static int raid_ctr(struct dm_target *ti, unsigned argc, char **argv)
>  	if (sector_div(chunks, chunk_size))
>  		goto err;
>  
> -	log = dm_dirty_log_create(log_argv[0], ti, sectors_per_dev,
> -				  NULL, log_cnt, log_argv+2);
> +	ti_len_sav = ti->len;
> +	ti->len = sectors_per_dev;
> +	log = dm_dirty_log_create(log_argv[0], ti, NULL, log_cnt, log_argv+2);
> +	ti->len = ti_len_sav;
>  	err = "Error creating dirty log";
>  	if (!log)
>  		goto err;
> diff --git a/include/linux/dm-dirty-log.h b/include/linux/dm-dirty-log.h
> index 641419f..7084503 100644
> --- a/include/linux/dm-dirty-log.h
> +++ b/include/linux/dm-dirty-log.h
> @@ -33,7 +33,6 @@ struct dm_dirty_log_type {
>  	struct list_head list;
>  
>  	int (*ctr)(struct dm_dirty_log *log, struct dm_target *ti,
> -		   sector_t log_size,
>  		   unsigned argc, char **argv);
>  	void (*dtr)(struct dm_dirty_log *log);
>  
> @@ -138,7 +137,7 @@ int dm_dirty_log_type_unregister(struct dm_dirty_log_type *type);
>   * type->constructor/destructor() directly.
>   */
>  struct dm_dirty_log *dm_dirty_log_create(const char *type_name,
> -			struct dm_target *ti, sector_t log_size,
> +			struct dm_target *ti,
>  			int (*flush_callback_fn)(struct dm_target *ti),
>  			unsigned argc, char **argv);
>  void dm_dirty_log_destroy(struct dm_dirty_log *log);
> 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel