raid5-cache fixes
cover raid5-cache fixes
Neil,
These are some fixes for raid5-cache. The first one fixes the issue related to trim. The last two make raid5-cache support log device IO error.
Thanks,
Shaohua
Shaohua Li (3):
raid5-cache: add trim support for log
md: don't export log device
raid5-cache: IO error handling
drivers/md/md.c | 4 ++-- drivers/md/raid5-cache.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 4 deletions(-)
Signature · 1 line
2.4.6
PATCH 1/3 raid5-cache: add trim support for log
Since superblock is updated infrequently, we do a simple trim of log disk (a synchronous trim)
--- drivers/md/raid5-cache.c | 44 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-)
drivers/md/raid5-cache.c +43 -1
@@ -654,6 +654,48 @@ 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 ; + +/* discard destroy old data in log, so force a super update */ + mddev = log->rdev->mddev; +/* +* mddev->thread could be shut down already in raid array stop. At that +* time, we should already lock reconfig_mutex +* */ +if (!mddev->thread) { +WARN_ON (!mddev_is_locked (mddev)); +md_update_sb (mddev,1 ); + }else { +set_bit (MD_CHANGE_PENDING , &mddev->flags); +md_wakeup_thread (mddev->thread); +wait_event (mddev->sb_wait, + !test_bit (MD_CHANGE_PENDING , &mddev->flags)); + } + +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 +751,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;
Signature · 1 line
2.4.6
Re: [PATCH 1/3] raid5-cache: add trim support for log
Quoted 32 lines · Shaohua Li
Since superblock is updated infrequently, we do a simple trim of log disk (a synchronous trim)
Signed-off-by Shaohua Li <[email protected]>--- drivers/md/raid5-cache.c | 44 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-)drivers/md/raid5-cache.c +16
-0@@ -654,6 +654,48 @@ 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 ; + +/* discard destroy old data in log, so force a super update */ + mddev = log->rdev->mddev; +/* +* mddev->thread could be shut down already in raid array stop. At that +* time, we should already lock reconfig_mutex
drivers/md/raid5-cache.c static void r5l_kick_io_unit(struct r5l_log *log) · line 682
Quoted 10 lines
drivers/md/raid5-cache.c +10
-0+* */ +if (!mddev->thread) { +WARN_ON (!mddev_is_locked (mddev)); +md_update_sb (mddev,1 ); + }else { +set_bit (MD_CHANGE_PENDING , &mddev->flags); +md_wakeup_thread (mddev->thread); +wait_event (mddev->sb_wait, + !test_bit (MD_CHANGE_PENDING , &mddev->flags)); + }
I didn't like this. It looks clumsy to me.
So I went to have a look at the code to understand why it was needed. I found that r5l_exit_log() was being called from free_conf(). I didn't notice that before.
->free() is only supposed to free, not write anything. It could be too late to write anything.
You need to get the raid5_quiesce(1) call to stablise the array. It can do the final r5l_do_reclaim().
So it is OK for free_conf to call r5l_exit_log as long as it only deregisters the thread and frees the data structures. The "r5l_do_reclaim" needs to be moved out.
I wonder where the md_update_sb() should go...
We currently calls "stop_writes" and then "mddev_detach".
So we shouldn't be writing anything by the time we get to mddev_detach, but in there we wait for writes to complete. That looks wrong.
I might move some of that stuff from mddev_detach to __md_stop_writes. Then md_stop_writes can call ->quiesce and then md_update_sb(). That is enough for raid5-cache to just call r5l_do_reclaim in raid5_quiesce, and have the md_update_sb() called at the right time.
I'll see if i can make that work.
Thanks,
NeilBrown
PATCH 2/3 md: don't export log device
In reply to Shaohua LiIf there is IO error in log device, don't export it like other raid disks. Otherwise we get kernel crash in different places since rdev->bdev, rdev->mddev becomes NULL
--- drivers/md/md.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/md/md.c +2 -2
@@ -2519,7 +2519,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)else err = -EBUSY ; }else if (cmd_match (buf,"remove" )) { -if (rdev->raid_disk >=0 ) +if (rdev->raid_disk >=0 ||test_bit (Journal, &rdev->flags)) err = -EBUSY ;else {struct mddev *mddev = rdev->mddev;
@@ -6040,7 +6040,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)clear_bit (Blocked, &rdev->flags);remove_and_add_spares (mddev, rdev); -if (rdev->raid_disk >=0 ) +if (rdev->raid_disk >=0 ||test_bit (Journal, &rdev->flags)) goto busy;if (mddev_is_clustered (mddev))
Signature · 1 line
2.4.6
Re: [PATCH 2/3] md: don't export log device
Quoted 33 lines · Shaohua Li
If there is IO error in log device, don't export it like other raid disks. Otherwise we get kernel crash in different places since
rdev->bdev,rdev->mddevbecomes NULLSigned-off-by Shaohua Li <[email protected]>--- drivers/md/md.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/md/md.c +2
-2@@ -2519,7 +2519,7 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)else err = -EBUSY ; }else if (cmd_match (buf,"remove" )) { -if (rdev->raid_disk >=0 ) +if (rdev->raid_disk >=0 ||test_bit (Journal, &rdev->flags)) err = -EBUSY ;else {struct mddev *mddev = rdev->mddev;@@ -6040,7 +6040,7 @@ static int hot_remove_disk(struct mddev *mddev, dev_t dev)clear_bit (Blocked, &rdev->flags);remove_and_add_spares (mddev, rdev); -if (rdev->raid_disk >=0 ) +if (rdev->raid_disk >=0 ||test_bit (Journal, &rdev->flags)) goto busy;if (mddev_is_clustered (mddev))Signature · 1 line
2.4.6
I'd rather keep these details local to the RAID5 code...
Can we just have raid5_remove_disk return -EBUSY when an attempt is made to remove the Journal device?
Thanks,
NeilBrown
PATCH 3/3 raid5-cache: IO error handling
In reply to Shaohua LiThere are 3 places the raid5-cache dispatches IO. The discard IO error doesn't matter, so we ignore it. The superblock write IO error can be handled in MD core. The remaining are log write and flush. When the IO error happens, we simply fail all raid disks and continue the stripe state machine. The MD/raid5 core can handle it (for example, mark all disks faulty, report bio error and so on).
--- drivers/md/raid5-cache.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
drivers/md/raid5-cache.c +17 -1
@@ -223,7 +223,16 @@ static void __r5l_set_io_unit_state(struct r5l_io_unit *io, io->state = state; } -/* XXX: totally ignores I/O errors */ +static void r5l_log_io_error (struct r5l_log *log) +{ +struct md_rdev *rdev; + +rcu_read_lock (); +rdev_for_each_rcu (rdev, log->rdev->mddev) +md_error (log->rdev->mddev, rdev); +rcu_read_unlock (); +} +static void r5l_log_endio (struct bio *bio) {struct r5l_io_unit *io = bio->bi_private;
@@ -232,6 +241,9 @@ static void r5l_log_endio(struct bio *bio)bio_put (bio); +if (bio->bi_error) +r5l_log_io_error (log); +if (!atomic_dec_and_test (&io->pending_io))return ;
@@ -594,6 +606,9 @@ static void r5l_log_flush_endio(struct bio *bio)struct r5l_io_unit *io;struct stripe_head *sh; +if (bio->bi_error) +r5l_log_io_error (log); +spin_lock_irqsave (&log->io_list_lock, flags);list_for_each_entry (io, &log->flushing_ios, log_sibling) {while (!list_empty (&io->stripe_list)) {
@@ -681,6 +696,7 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log, !test_bit(MD_CHANGE_PENDING, &mddev->flags)); } +/* discard IO error really doesn't matter, ignore it */ if (log->last_checkpoint < end) { blkdev_issue_discard(bdev, log->last_checkpoint + log->rdev->data_offset,
Signature · 1 line
2.4.6
Re: [PATCH 3/3] raid5-cache: IO error handling
Quoted 20 lines · Shaohua Li
There are 3 places the raid5-cache dispatches IO. The discard IO error doesn't matter, so we ignore it. The superblock write IO error can be handled in MD core. The remaining are log write and flush. When the IO error happens, we simply fail all raid disks and continue the stripe state machine. The MD/raid5 core can handle it (for example, mark all disks faulty, report bio error and so on).
Signed-off-by Shaohua Li <[email protected]>--- drivers/md/raid5-cache.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)drivers/md/raid5-cache.c +0
-0@@ -223,7 +223,16 @@ static void __r5l_set_io_unit_state(struct r5l_io_unit *io, io->state = state; }
drivers/md/raid5-cache.c static void __r5l_set_io_unit_state(struct r5l_io_unit *io, · line 234
Quoted 10 lines
drivers/md/raid5-cache.c +9
-1-/* XXX: totally ignores I/O errors */ +static void r5l_log_io_error (struct r5l_log *log) +{ +struct md_rdev *rdev; + +rcu_read_lock (); +rdev_for_each_rcu (rdev, log->rdev->mddev) +md_error (log->rdev->mddev, rdev); +rcu_read_unlock (); +}
This fails spare devices too... seems a bit heavy handed.
If the journal device fails we should still be able to read from the array, just not write.
So can we just enhance the
if (s.failed > conf->max_degraded) {test in handle_stripe(), and probably improve has_failed() too??
Thanks,
NeilBrown
Quoted 34 lines
drivers/md/raid5-cache.c +8
-0+static void r5l_log_endio (struct bio *bio) {struct r5l_io_unit *io = bio->bi_private;@@ -232,6 +241,9 @@ static void r5l_log_endio(struct bio *bio)bio_put (bio); +if (bio->bi_error) +r5l_log_io_error (log); +if (!atomic_dec_and_test (&io->pending_io))return ;@@ -594,6 +606,9 @@ static void r5l_log_flush_endio(struct bio *bio)struct r5l_io_unit *io;struct stripe_head *sh; +if (bio->bi_error) +r5l_log_io_error (log); +spin_lock_irqsave (&log->io_list_lock, flags);list_for_each_entry (io, &log->flushing_ios, log_sibling) {while (!list_empty (&io->stripe_list)) {@@ -681,6 +696,7 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log, !test_bit(MD_CHANGE_PENDING, &mddev->flags)); } +/* discard IO error really doesn't matter, ignore it */ if (log->last_checkpoint < end) { blkdev_issue_discard(bdev, log->last_checkpoint + log->rdev->data_offset,Signature · 1 line
2.4.6
Re: [PATCH 3/3] raid5-cache: IO error handling
In reply to Shaohua LiQuoted 6 lines · Shaohua Li
There are 3 places the raid5-cache dispatches IO. The discard IO error doesn't matter, so we ignore it. The superblock write IO error can be handled in MD core. The remaining are log write and flush. When the IO error happens, we simply fail all raid disks and continue the stripe state machine. The MD/raid5 core can handle it (for example, mark all disks faulty, report bio error and so on).
This introduces a use after free, which will always report an I/O error when SLAB poisoning is enabled. The following patch needs to be folded into it to fix that:
drivers/md/raid5-cache.c +2 -2
@@ -239,11 +239,11 @@ static void r5l_log_endio(struct bio *bio)struct r5l_log *log = io->log;unsigned long flags; -bio_put (bio); -if (bio->bi_error)r5l_log_io_error (log); +bio_put (bio); +if (!atomic_dec_and_test (&io->pending_io))return ;