Re: [PATCH 1/2] raid5-cache: update superblock at shutdown/reboot

[NeilBrown <neilb@suse.com>]

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

On Thu, Nov 17 2016, Shaohua Li wrote:

> Currently raid5-cache update superblock in .quiesce. But since at
> shutdown/reboot, .quiesce is called with reconfig mutex locked,
> superblock isn't guaranteed to be called in reclaim thread (see
> 8e018c21da3). This will make assemble do unnecessary journal recovery.
> It doesn't corrupt data but is annoying.  This adds an extra hook to
> guarantee journal is flushed to raid disks.  And since this hook is
> called before superblock update, this will guarantee we have a uptodate
> superblock in shutdown/reboot

Hi.
I don't quite follow some of the reasoning here.
In particular, the ->stop_writes() that you have implemented
does almost exactly the same thing as r5l_quiesce(1).
So why not simply call ->quiesce(mddev, 1) in __md_stop_writes()??
You probably need to also call ->quiesce(mddev, 0) to keep things
balanced.

Also you have introduced a static mutex (which isn't my favourite sort
of thing) without giving any explanation why in the changelog comment.
So I cannot easily see if that addition is at all justified.

Thanks,
NeilBrown

>
> Signed-off-by: Shaohua Li <shli@fb.com>
> Cc: Zhengyuan Liu <liuzhengyuan@kylinos.cn>
> Cc: NeilBrown <neilb@suse.com>
> ---
>  drivers/md/md.c          |  3 +++
>  drivers/md/md.h          |  2 ++
>  drivers/md/raid5-cache.c | 19 ++++++++++++++++++-
>  drivers/md/raid5.c       |  3 +++
>  drivers/md/raid5.h       |  1 +
>  5 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 1f1c7f0..155dd42 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5471,6 +5471,9 @@ static void __md_stop_writes(struct mddev *mddev)
>  
>  	del_timer_sync(&mddev->safemode_timer);
>  
> +	if (mddev->pers && mddev->pers->stop_writes)
> +		mddev->pers->stop_writes(mddev);
> +
>  	bitmap_flush(mddev);
>  
>  	if (mddev->ro == 0 &&
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index af6b33c..8da6fe9 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -512,6 +512,8 @@ struct md_personality
>  	/* congested implements bdi.congested_fn().
>  	 * Will not be called while array is 'suspended' */
>  	int (*congested)(struct mddev *mddev, int bits);
> +	/* stop touching raid disks */
> +	void (*stop_writes)(struct mddev *mddev);
>  };
>  
>  struct md_sysfs_entry {
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 2e270e6..641dec8 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -738,11 +738,13 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log,
>  
>  static void r5l_do_reclaim(struct r5l_log *log)
>  {
> +	static DEFINE_MUTEX(lock);
>  	sector_t reclaim_target = xchg(&log->reclaim_target, 0);
>  	sector_t reclaimable;
>  	sector_t next_checkpoint;
>  	u64 next_cp_seq;
>  
> +	mutex_lock(&lock);
>  	spin_lock_irq(&log->io_list_lock);
>  	/*
>  	 * move proper io_unit to reclaim list. We should not change the order.
> @@ -769,8 +771,10 @@ static void r5l_do_reclaim(struct r5l_log *log)
>  	spin_unlock_irq(&log->io_list_lock);
>  
>  	BUG_ON(reclaimable < 0);
> -	if (reclaimable == 0)
> +	if (reclaimable == 0) {
> +		mutex_unlock(&lock);
>  		return;
> +	}
>  
>  	/*
>  	 * write_super will flush cache of each raid disk. We must write super
> @@ -784,6 +788,8 @@ static void r5l_do_reclaim(struct r5l_log *log)
>  	log->last_cp_seq = next_cp_seq;
>  	mutex_unlock(&log->io_mutex);
>  
> +	mutex_unlock(&lock);
> +
>  	r5l_run_no_space_stripes(log);
>  }
>  
> @@ -836,6 +842,17 @@ void r5l_quiesce(struct r5l_log *log, int state)
>  	}
>  }
>  
> +void r5l_stop_writes(struct mddev *mddev)
> +{
> +	struct r5conf *conf = mddev->private;
> +	struct r5l_log *log = conf->log;
> +
> +	if (!log)
> +		return;
> +	r5l_wake_reclaim(log, -1L);
> +	r5l_do_reclaim(log);
> +}
> +
>  bool r5l_log_disk_error(struct r5conf *conf)
>  {
>  	struct r5l_log *log;
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index df88656..3d27338 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -7866,6 +7866,7 @@ static struct md_personality raid6_personality =
>  	.quiesce	= raid5_quiesce,
>  	.takeover	= raid6_takeover,
>  	.congested	= raid5_congested,
> +	.stop_writes	= r5l_stop_writes,
>  };
>  static struct md_personality raid5_personality =
>  {
> @@ -7889,6 +7890,7 @@ static struct md_personality raid5_personality =
>  	.quiesce	= raid5_quiesce,
>  	.takeover	= raid5_takeover,
>  	.congested	= raid5_congested,
> +	.stop_writes	= r5l_stop_writes,
>  };
>  
>  static struct md_personality raid4_personality =
> @@ -7913,6 +7915,7 @@ static struct md_personality raid4_personality =
>  	.quiesce	= raid5_quiesce,
>  	.takeover	= raid4_takeover,
>  	.congested	= raid5_congested,
> +	.stop_writes	= r5l_stop_writes,
>  };
>  
>  static int __init raid5_init(void)
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 57ec49f..0643cc0 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -633,4 +633,5 @@ extern void r5l_stripe_write_finished(struct stripe_head *sh);
>  extern int r5l_handle_flush_request(struct r5l_log *log, struct bio *bio);
>  extern void r5l_quiesce(struct r5l_log *log, int state);
>  extern bool r5l_log_disk_error(struct r5conf *conf);
> +extern void r5l_stop_writes(struct mddev *mddev);
>  #endif
> -- 
> 2.9.3

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