Re: [PATCH 3/6] raid5-cache: add trim support for log

[Neil Brown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

Shaohua Li <shli@fb.com> writes:

> Since superblock is updated infrequently, we do a simple trim of log
> disk (a synchronous trim)
>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  drivers/md/raid5-cache.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 93097a8..6c52168 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -654,6 +654,42 @@ static void r5l_kick_io_unit(struct r5l_log *log)
>  }
>  
>  static void r5l_write_super(struct r5l_log *log, sector_t cp);
> +static void r5l_write_super_and_discard_space(struct r5l_log *log,
> +	sector_t end)
> +{
> +	struct block_device *bdev = log->rdev->bdev;
> +	struct mddev *mddev;
> +
> +	r5l_write_super(log, end);
> +
> +	if (!blk_queue_discard(bdev_get_queue(bdev)))
> +		return;
> +
> +	mddev = log->rdev->mddev;
> +	if (!mddev_is_locked(mddev)) {
> +		set_bit(MD_CHANGE_PENDING, &mddev->flags);
> +		md_wakeup_thread(mddev->thread);
> +		wait_event(mddev->sb_wait,
> +			   !test_bit(MD_CHANGE_PENDING, &mddev->flags));
> +	} else { /* we are stopping the array, already take reconfig_mutex */
> +		md_update_sb(mddev, 1);
> +	}

No.

Just because mddev is locked, that doesn't mean that this thread owns
the lock.  Some other thread might just happen to have it locked for a
moment, and may unlock it long before we get to md_update_sb().

If you cannot block here, then you need to find a way to schedule the
discard after the md_update_sb() has happened.
Maybe set some flag, and in raid5d(), after md_check_recovery(), see if
the superblock has been written and if the discard is still pending, and
then do the discard. Maybe.

Or pass a flag into r5l_do_reclaim() to say whether this thread holds
the lock or not.

NeilBrown

> +
> +	if (log->last_checkpoint < end) {
> +		blkdev_issue_discard(bdev,
> +				log->last_checkpoint + log->rdev->data_offset,
> +				end - log->last_checkpoint, GFP_NOIO, 0);
> +	} else {
> +		blkdev_issue_discard(bdev,
> +				log->last_checkpoint + log->rdev->data_offset,
> +				log->device_size - log->last_checkpoint,
> +				GFP_NOIO, 0);
> +		blkdev_issue_discard(bdev, log->rdev->data_offset, end,
> +				GFP_NOIO, 0);
> +	}
> +}
> +
> +
>  static void r5l_do_reclaim(struct r5l_log *log)
>  {
>  	struct r5l_io_unit *io, *last;
> @@ -709,7 +745,7 @@ static void r5l_do_reclaim(struct r5l_log *log)
>  	 * here, because the log area might be reused soon and we don't want to
>  	 * confuse recovery
>  	 */
> -	r5l_write_super(log, last->log_start);
> +	r5l_write_super_and_discard_space(log, last->log_start);
>  
>  	mutex_lock(&log->io_mutex);
>  	log->last_checkpoint = last->log_start;
> -- 
> 2.4.6

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]