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@xxxxxx>
> Cc: Zhengyuan Liu <liuzhengyuan@xxxxxxxxxx>
> Cc: NeilBrown <neilb@xxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
