Re: [PATCH for-dm-3.14-fixes 3/8] dm thin: set flag if metadata is out of space

[Joe Thornber <thornber@redhat.com>]

NACK.

It's odd that the interface has dm-thin telling dm-thin-metadata
whether or not it's out of space (yet thin-metadata clears it
itself).  So I don't like the patch from that point of view, though
it's a minor quibble.

More importantly, running out of metadata space is a serious error
that causes a transaction to be aborted.  Any thins that contained
metadata changes in that aborted transaction need to be umounted and
fscked.  We should not transistion back to WRITE mode just because
more metadata space is provided since this doesn't force the sys admin
to repare/check the thins that have lost io.

So I'm nacking because I think you're planning to special case running
out of metadata space, rather than treating it the same as any other
metadata error (eg, a failed write to the metadata device).

As we've discussed the correct thing to do when getting any error from
a metadata operation is:

  i) Abort the current transaction

  ii) flick to READ_ONLY mode.  Any thin devices that lost writes due
      to the abort will reflect this by erroring every subsequent
      REQ_FUA or REQ_FLUSH.

  iii) Set a needs_check flag in the metadata to prevent a table
       reload transitioning to WRITE mode.

- Joe

On Thu, Feb 20, 2014 at 09:56:00PM -0500, Mike Snitzer wrote:
> It is difficult for the metadata space map to accurately account for its
> free space.  Manually establish a flag that reflects this out of space
> condition -- until/unless dm_pool_get_free_metadata_block_count() can
> return a known out of space value (either 0 or some fixed reserve
> threshold we create).
> 
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> ---
>  drivers/md/dm-thin-metadata.c | 33 ++++++++++++++++++++++++++++++++-
>  drivers/md/dm-thin-metadata.h |  7 +++++++
>  drivers/md/dm-thin.c          |  8 +++-----
>  3 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
> index 3bb4506..21f3998 100644
> --- a/drivers/md/dm-thin-metadata.c
> +++ b/drivers/md/dm-thin-metadata.c
> @@ -192,6 +192,15 @@ struct dm_pool_metadata {
>  	 * operation possible in this state is the closing of the device.
>  	 */
>  	bool fail_io:1;
> +
> +	/*
> +	 * Set if metadata space has been exhausted.  It is difficult for the
> +	 * metadata space map to accurately account for its free space.  So
> +	 * until/unless dm_pool_get_free_metadata_block_count can return a known
> +	 * out of space value (either 0 or some fixed reserve threshold we create)
> +	 * manually establish a flag that reflects this out of space condition.
> +	 */
> +	bool metadata_out_of_space:1;
>  };
>  
>  struct dm_thin_device {
> @@ -815,6 +824,7 @@ struct dm_pool_metadata *dm_pool_metadata_open(struct block_device *bdev,
>  	INIT_LIST_HEAD(&pmd->thin_devices);
>  	pmd->read_only = false;
>  	pmd->fail_io = false;
> +	pmd->metadata_out_of_space = false;
>  	pmd->bdev = bdev;
>  	pmd->data_block_size = data_block_size;
>  
> @@ -1603,6 +1613,24 @@ int dm_pool_get_free_metadata_block_count(struct dm_pool_metadata *pmd,
>  	return r;
>  }
>  
> +void dm_pool_set_metadata_out_of_space(struct dm_pool_metadata *pmd)
> +{
> +	down_write(&pmd->root_lock);
> +	pmd->metadata_out_of_space = true;
> +	up_write(&pmd->root_lock);
> +}
> +
> +bool dm_pool_is_metadata_out_of_space(struct dm_pool_metadata *pmd)
> +{
> +	bool r;
> +
> +	down_read(&pmd->root_lock);
> +	r = pmd->metadata_out_of_space;
> +	up_read(&pmd->root_lock);
> +
> +	return r;
> +}
> +
>  int dm_pool_get_metadata_dev_size(struct dm_pool_metadata *pmd,
>  				  dm_block_t *result)
>  {
> @@ -1719,8 +1747,11 @@ int dm_pool_resize_metadata_dev(struct dm_pool_metadata *pmd, dm_block_t new_cou
>  	int r = -EINVAL;
>  
>  	down_write(&pmd->root_lock);
> -	if (!pmd->fail_io)
> +	if (!pmd->fail_io) {
>  		r = __resize_space_map(pmd->metadata_sm, new_count);
> +		if (!r)
> +			pmd->metadata_out_of_space = false;
> +	}
>  	up_write(&pmd->root_lock);
>  
>  	return r;
> diff --git a/drivers/md/dm-thin-metadata.h b/drivers/md/dm-thin-metadata.h
> index dd6088a..cc78fbb 100644
> --- a/drivers/md/dm-thin-metadata.h
> +++ b/drivers/md/dm-thin-metadata.h
> @@ -91,6 +91,11 @@ int dm_pool_commit_metadata(struct dm_pool_metadata *pmd);
>  int dm_pool_abort_metadata(struct dm_pool_metadata *pmd);
>  
>  /*
> + * Set if metadata space has been exhausted.
> + */
> +void dm_pool_set_metadata_out_of_space(struct dm_pool_metadata *pmd);
> +
> +/*
>   * Set/get userspace transaction id.
>   */
>  int dm_pool_set_metadata_transaction_id(struct dm_pool_metadata *pmd,
> @@ -185,6 +190,8 @@ int dm_pool_get_data_dev_size(struct dm_pool_metadata *pmd, dm_block_t *result);
>  
>  int dm_pool_block_is_used(struct dm_pool_metadata *pmd, dm_block_t b, bool *result);
>  
> +bool dm_pool_is_metadata_out_of_space(struct dm_pool_metadata *pmd);
> +
>  /*
>   * Returns -ENOSPC if the new size is too small and already allocated
>   * blocks would be lost.
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index 42d08eb..8e68831 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -1464,16 +1464,14 @@ static void out_of_data_space(struct pool *pool)
>  
>  static void metadata_operation_failed(struct pool *pool, const char *op, int r)
>  {
> -	dm_block_t free_blocks;
> -
>  	DMERR_LIMIT("%s: metadata operation '%s' failed: error = %d",
>  		    dm_device_name(pool->pool_md), op, r);
>  
> -	if (r == -ENOSPC &&
> -	    !dm_pool_get_free_metadata_block_count(pool->pmd, &free_blocks) &&
> -	    !free_blocks)
> +	if (r == -ENOSPC) {
> +		dm_pool_set_metadata_out_of_space(pool->pmd);
>  		DMERR_LIMIT("%s: no free metadata space available.",
>  			    dm_device_name(pool->pool_md));
> +	}
>  
>  	set_pool_mode(pool, PM_READ_ONLY);
>  }
> -- 
> 1.8.3.1
>