On Tue, Mar 08 2011 at 1:51am -0500,
Martin K. Petersen <mkp@xxxxxxx> wrote:
> >>>>> "Mike" == Mike Snitzer <snitzer@xxxxxxxxxx> writes:
>
> Mike> Thanks for sorting this out. Can you post a patch that has a
> Mike> proper header with your Signed-off-by? Also including Zdenek's
> Mike> Tested-by, and my:
>
> I contemplated a bit and decided I liked the following approach
> better. What do you think?
>
>
> block: Require subsystems to explicitly allocate bio_set integrity mempool
>
> MD and DM create a new bio_set for every metadevice. Each bio_set has an
> integrity mempool attached regardless of whether the metadevice is
> capable of passing integrity metadata. This is a waste of memory.
>
> Instead we defer the allocation decision to MD and DM since we know at
> metadevice creation time whether integrity passthrough is needed or not.
>
> Automatic integrity mempool allocation can then be removed from
> bioset_create() and we make an explicit integrity allocation for the
> fs_bio_set.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
> Reported-by: Zdenek Kabelac <zkabelac@xxxxxxxxxx>
In general this approach looks fine too. Doesn't address the concern I
had yesterday about the blk_integrity block_size relative to DM (load vs
resume) but I'd imagine you'd like to sort that out in a separate patch.
But I found a few DM issues below. Provided those are sorted out:
Acked-by: Mike Snitzer <snitzer@xxxxxxxxxx>
(I'll defer to Jens and Neil for the block and MD bits)
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index eaa3af0..b55ef95 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -2643,9 +2643,10 @@ int dm_noflush_suspending(struct dm_target *ti)
> }
> EXPORT_SYMBOL_GPL(dm_noflush_suspending);
>
> -struct dm_md_mempools *dm_alloc_md_mempools(unsigned type)
> +struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned int integrity)
> {
Any reason you didn't just use 'unsigned integrity' like you did below
in the dm.h prototype?
> struct dm_md_mempools *pools = kmalloc(sizeof(*pools), GFP_KERNEL);
> + unsigned int pool_size = (type == DM_TYPE_BIO_BASED) ? 16 : MIN_IOS;
>
> if (!pools)
> return NULL;
> @@ -2662,11 +2663,13 @@ struct dm_md_mempools *dm_alloc_md_mempools(unsigned type)
> if (!pools->tio_pool)
> goto free_io_pool_and_out;
>
> - pools->bs = (type == DM_TYPE_BIO_BASED) ?
> - bioset_create(16, 0) : bioset_create(MIN_IOS, 0);
> + pools->bs = bioset_create(pool_size, 0);
> if (!pools->bs)
> goto free_tio_pool_and_out;
>
> + if (integrity && bioset_integrity_create(pools->bs, pool_size))
> + goto free_tio_pool_and_out;
> +
> return pools;
>
> free_tio_pool_and_out:
Don't you need to bioset_free(pools->bs) if bioset_integrity_create()
fails? So you'd need a new 'free_bioset_and_out' label. Also seems
like maybe you had some extra whitespace on the goto?
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index 0c2dd5f..1aaf167 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -149,7 +149,7 @@ void dm_kcopyd_exit(void);
> /*
> * Mempool operations
> */
> -struct dm_md_mempools *dm_alloc_md_mempools(unsigned type);
> +struct dm_md_mempools *dm_alloc_md_mempools(unsigned type, unsigned integrity);
> void dm_free_md_mempools(struct dm_md_mempools *pools);
>
> #endif
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
