* [PATCH 01/11] raid5-cache: free I/O units earlier
From: Christoph Hellwig @ 2015-10-05 7:31 UTC (permalink / raw)
To: Shaohua Li, neilb; +Cc: dan.j.williams, Kernel-team, linux-raid
In-Reply-To: <1444030276-30850-1-git-send-email-hch@lst.de>
There is no good reason to keep the I/O unit structures around after the
stripe has been written back to the RAID array. The only information
we need is the log sequence number, and the checkpoint offset of the
highest successfull writeback. Store those in the log structure, and
free the IO units from __r5l_stripe_write_finished.
Besides simplifying the code this also avoid having to keep the allocation
for the I/O unit around for a potentially long time as superblock updates
that checkpoint the log do not happen very often.
This also fixes the previously incorrect calculation of 'free' in
r5l_do_reclaim as a side effect: previous if took the last unit which
isn't checkpointed into account.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/md/raid5-cache.c | 144 ++++++++++++++++++-----------------------------
1 file changed, 55 insertions(+), 89 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index e917e53..5b5f346 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -51,6 +51,9 @@ struct r5l_log {
sector_t log_start; /* log head. where new data appends */
u64 seq; /* log head sequence */
+ sector_t next_checkpoint;
+ u64 next_cp_seq;
+
struct mutex io_mutex;
struct r5l_io_unit *current_io; /* current io_unit accepting new data */
@@ -65,9 +68,6 @@ struct r5l_log {
* cache flush */
struct list_head flushed_ios; /* io_units which settle down in log disk */
struct bio flush_bio;
- struct list_head stripe_end_ios;/* io_units which have been completely
- * written to the RAID but have not yet
- * been considered for updating super */
struct kmem_cache *io_kc;
@@ -186,35 +186,6 @@ static void r5l_move_io_unit_list(struct list_head *from, struct list_head *to,
}
}
-/*
- * We don't want too many io_units reside in stripe_end_ios list, which will
- * waste a lot of memory. So we try to remove some. But we must keep at least 2
- * io_units. The superblock must point to a valid meta, if it's the last meta,
- * recovery can scan less
- */
-static void r5l_compress_stripe_end_list(struct r5l_log *log)
-{
- struct r5l_io_unit *first, *last, *io;
-
- first = list_first_entry(&log->stripe_end_ios,
- struct r5l_io_unit, log_sibling);
- last = list_last_entry(&log->stripe_end_ios,
- struct r5l_io_unit, log_sibling);
- if (first == last)
- return;
- list_del(&first->log_sibling);
- list_del(&last->log_sibling);
- while (!list_empty(&log->stripe_end_ios)) {
- io = list_first_entry(&log->stripe_end_ios,
- struct r5l_io_unit, log_sibling);
- list_del(&io->log_sibling);
- first->log_end = io->log_end;
- r5l_free_io_unit(log, io);
- }
- list_add_tail(&first->log_sibling, &log->stripe_end_ios);
- list_add_tail(&last->log_sibling, &log->stripe_end_ios);
-}
-
static void __r5l_set_io_unit_state(struct r5l_io_unit *io,
enum r5l_io_unit_state state)
{
@@ -548,31 +519,53 @@ static void r5l_run_no_space_stripes(struct r5l_log *log)
spin_unlock(&log->no_space_stripes_lock);
}
+static sector_t r5l_reclaimable_space(struct r5l_log *log)
+{
+ return r5l_ring_distance(log, log->last_checkpoint,
+ log->next_checkpoint);
+}
+
+static bool r5l_complete_flushed_ios(struct r5l_log *log)
+{
+ struct r5l_io_unit *io, *next;
+ bool found = false;
+
+ assert_spin_locked(&log->io_list_lock);
+
+ list_for_each_entry_safe(io, next, &log->flushed_ios, log_sibling) {
+ /* don't change list order */
+ if (io->state < IO_UNIT_STRIPE_END)
+ break;
+
+ log->next_checkpoint = io->log_start;
+ log->next_cp_seq = io->seq;
+
+ list_del(&io->log_sibling);
+ r5l_free_io_unit(log, io);
+
+ found = true;
+ }
+
+
+ return found;
+}
+
static void __r5l_stripe_write_finished(struct r5l_io_unit *io)
{
struct r5l_log *log = io->log;
- struct r5l_io_unit *last;
- sector_t reclaimable_space;
unsigned long flags;
spin_lock_irqsave(&log->io_list_lock, flags);
__r5l_set_io_unit_state(io, IO_UNIT_STRIPE_END);
- /* might move 0 entry */
- r5l_move_io_unit_list(&log->flushed_ios, &log->stripe_end_ios,
- IO_UNIT_STRIPE_END);
- if (list_empty(&log->stripe_end_ios)) {
+
+ if (!r5l_complete_flushed_ios(log)) {
spin_unlock_irqrestore(&log->io_list_lock, flags);
return;
}
- last = list_last_entry(&log->stripe_end_ios,
- struct r5l_io_unit, log_sibling);
- reclaimable_space = r5l_ring_distance(log, log->last_checkpoint,
- last->log_end);
- if (reclaimable_space >= log->max_free_space)
+ if (r5l_reclaimable_space(log) > log->max_free_space)
r5l_wake_reclaim(log, 0);
- r5l_compress_stripe_end_list(log);
spin_unlock_irqrestore(&log->io_list_lock, flags);
wake_up(&log->iounit_wait);
}
@@ -651,13 +644,6 @@ void r5l_flush_stripe_to_raid(struct r5l_log *log)
submit_bio(WRITE_FLUSH, &log->flush_bio);
}
-static void r5l_kick_io_unit(struct r5l_log *log)
-{
- md_wakeup_thread(log->rdev->mddev->thread);
- wait_event_lock_irq(log->iounit_wait, !list_empty(&log->stripe_end_ios),
- log->io_list_lock);
-}
-
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)
@@ -698,10 +684,10 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log,
static void r5l_do_reclaim(struct r5l_log *log)
{
- struct r5l_io_unit *io, *last;
- LIST_HEAD(list);
- sector_t free = 0;
sector_t reclaim_target = xchg(&log->reclaim_target, 0);
+ sector_t reclaimable;
+ sector_t next_checkpoint;
+ u64 next_cp_seq;
spin_lock_irq(&log->io_list_lock);
/*
@@ -710,60 +696,41 @@ static void r5l_do_reclaim(struct r5l_log *log)
* shouldn't reuse space of an unreclaimable io_unit
*/
while (1) {
- struct list_head *target_list = NULL;
-
- while (!list_empty(&log->stripe_end_ios)) {
- io = list_first_entry(&log->stripe_end_ios,
- struct r5l_io_unit, log_sibling);
- list_move_tail(&io->log_sibling, &list);
- free += r5l_ring_distance(log, io->log_start,
- io->log_end);
- }
-
- if (free >= reclaim_target ||
+ reclaimable = r5l_reclaimable_space(log);
+ if (reclaimable >= reclaim_target ||
(list_empty(&log->running_ios) &&
list_empty(&log->io_end_ios) &&
list_empty(&log->flushing_ios) &&
list_empty(&log->flushed_ios)))
break;
- /* Below waiting mostly happens when we shutdown the raid */
- if (!list_empty(&log->flushed_ios))
- target_list = &log->flushed_ios;
- else if (!list_empty(&log->flushing_ios))
- target_list = &log->flushing_ios;
- else if (!list_empty(&log->io_end_ios))
- target_list = &log->io_end_ios;
- else if (!list_empty(&log->running_ios))
- target_list = &log->running_ios;
-
- r5l_kick_io_unit(log);
+ md_wakeup_thread(log->rdev->mddev->thread);
+ wait_event_lock_irq(log->iounit_wait,
+ r5l_reclaimable_space(log) > reclaimable,
+ log->io_list_lock);
}
+
+ next_checkpoint = log->next_checkpoint;
+ next_cp_seq = log->next_cp_seq;
spin_unlock_irq(&log->io_list_lock);
- if (list_empty(&list))
+ BUG_ON(reclaimable < 0);
+ if (reclaimable == 0)
return;
- /* super always point to last valid meta */
- last = list_last_entry(&list, struct r5l_io_unit, log_sibling);
/*
* write_super will flush cache of each raid disk. We must write super
* here, because the log area might be reused soon and we don't want to
* confuse recovery
*/
- r5l_write_super_and_discard_space(log, last->log_start);
+ r5l_write_super_and_discard_space(log, next_checkpoint);
mutex_lock(&log->io_mutex);
- log->last_checkpoint = last->log_start;
- log->last_cp_seq = last->seq;
+ log->last_checkpoint = next_checkpoint;
+ log->last_cp_seq = next_cp_seq;
mutex_unlock(&log->io_mutex);
- r5l_run_no_space_stripes(log);
- while (!list_empty(&list)) {
- io = list_first_entry(&list, struct r5l_io_unit, log_sibling);
- list_del(&io->log_sibling);
- r5l_free_io_unit(log, io);
- }
+ r5l_run_no_space_stripes(log);
}
static void r5l_reclaim_thread(struct md_thread *thread)
@@ -1153,7 +1120,6 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
spin_lock_init(&log->io_list_lock);
INIT_LIST_HEAD(&log->running_ios);
INIT_LIST_HEAD(&log->io_end_ios);
- INIT_LIST_HEAD(&log->stripe_end_ios);
INIT_LIST_HEAD(&log->flushing_ios);
INIT_LIST_HEAD(&log->flushed_ios);
bio_init(&log->flush_bio);
--
1.9.1
^ permalink raw reply related
* raid5-cache I/O path improvements V3
From: Christoph Hellwig @ 2015-10-05 7:31 UTC (permalink / raw)
To: Shaohua Li, neilb; +Cc: dan.j.williams, Kernel-team, linux-raid
Hi Shaohua, hi Neil,
this series contains a few updates to the raid5-cache feature, and goes
on top of dm/devel + the series Shaohua sent yesterday.
Besides the rebase the only changes compared to the previous version
is that we only bypasss the batch and flush state for devices that
do not have a volatile cache at all like persistent memory or high
performance flash devices.
^ permalink raw reply
* [PATCH 6/6] raid5-cache: IO error handling
From: Shaohua Li @ 2015-10-04 16:20 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443973492.git.shli@fb.com>
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 mark log disk faulty and fail all write IO. Read IO is
still allowed to run. Userspace will get a notification too and
corresponding daemon can choose setting raid array readonly for example.
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/raid5-cache.c | 15 ++++++++++++++-
drivers/md/raid5.c | 4 +++-
drivers/md/raid5.h | 2 ++
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 6c52168..e917e53 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -223,13 +223,15 @@ 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_endio(struct bio *bio)
{
struct r5l_io_unit *io = bio->bi_private;
struct r5l_log *log = io->log;
unsigned long flags;
+ if (bio->bi_error)
+ md_error(log->rdev->mddev, log->rdev);
+
bio_put(bio);
if (!atomic_dec_and_test(&io->pending_io))
@@ -594,6 +596,9 @@ static void r5l_log_flush_endio(struct bio *bio)
struct r5l_io_unit *io;
struct stripe_head *sh;
+ if (bio->bi_error)
+ md_error(log->rdev->mddev, log->rdev);
+
spin_lock_irqsave(&log->io_list_lock, flags);
list_for_each_entry(io, &log->flushing_ios, log_sibling) {
while (!list_empty(&io->stripe_list)) {
@@ -675,6 +680,7 @@ static void r5l_write_super_and_discard_space(struct r5l_log *log,
md_update_sb(mddev, 1);
}
+ /* 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,
@@ -802,6 +808,13 @@ void r5l_quiesce(struct r5l_log *log, int state)
}
}
+bool r5l_log_disk_error(struct r5l_log *log)
+{
+ if (!log)
+ return false;
+ return !test_bit(In_sync, &log->rdev->flags);
+}
+
struct r5l_recovery_ctx {
struct page *meta_page; /* current meta */
sector_t meta_total_blocks; /* total size of current meta and data */
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index c164501..53bb7f6 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3147,6 +3147,7 @@ handle_failed_stripe(struct r5conf *conf, struct stripe_head *sh,
* the data has not reached the cache yet.
*/
if (!test_bit(R5_Wantfill, &sh->dev[i].flags) &&
+ s->failed > conf->max_degraded &&
(!test_bit(R5_Insync, &sh->dev[i].flags) ||
test_bit(R5_ReadError, &sh->dev[i].flags))) {
spin_lock_irq(&sh->stripe_lock);
@@ -4015,6 +4016,7 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
s->expanded = test_bit(STRIPE_EXPAND_READY, &sh->state) && !sh->batch_head;
s->failed_num[0] = -1;
s->failed_num[1] = -1;
+ s->log_failed = r5l_log_disk_error(conf->log);
/* Now to look around and see what can be done */
rcu_read_lock();
@@ -4358,7 +4360,7 @@ static void handle_stripe(struct stripe_head *sh)
/* check if the array has lost more than max_degraded devices and,
* if so, some requests might need to be failed.
*/
- if (s.failed > conf->max_degraded) {
+ if (s.failed > conf->max_degraded || s.log_failed) {
sh->check_state = 0;
sh->reconstruct_state = 0;
break_stripe_batch_list(sh, 0);
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 1ab534c..d114389 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -272,6 +272,7 @@ struct stripe_head_state {
struct bio_list return_bi;
struct md_rdev *blocked_rdev;
int handle_bad_blocks;
+ int log_failed;
};
/* Flags for struct r5dev.flags */
@@ -631,4 +632,5 @@ extern void r5l_flush_stripe_to_raid(struct r5l_log *log);
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 r5l_log *log);
#endif
--
2.4.6
^ permalink raw reply related
* [PATCH 5/6] md: set In_Sync for log disk
From: Shaohua Li @ 2015-10-04 16:20 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443973492.git.shli@fb.com>
We forgot set the bit for log disk
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/md.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index ec6574d..2fbecd7 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -1646,6 +1646,7 @@ static int super_1_validate(struct mddev *mddev, struct md_rdev *rdev)
return -EINVAL;
}
set_bit(Journal, &rdev->flags);
+ set_bit(In_sync, &rdev->flags);
rdev->journal_tail = le64_to_cpu(sb->journal_tail);
if (mddev->recovery_cp == MaxSector)
set_bit(MD_JOURNAL_CLEAN, &mddev->flags);
--
2.4.6
^ permalink raw reply related
* [PATCH 4/6] md: don't export log device
From: Shaohua Li @ 2015-10-04 16:20 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443973492.git.shli@fb.com>
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->mddev becomes NULL
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/md.c | 4 ++--
drivers/md/raid5.c | 2 ++
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index c643c9a..ec6574d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2523,7 +2523,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;
@@ -6044,7 +6044,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))
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 216fa3c..c164501 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7128,6 +7128,8 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
struct disk_info *p = conf->disks + number;
print_raid5_conf(conf);
+ if (test_bit(Journal, &rdev->flags))
+ return -EBUSY;
if (rdev == p->rdev)
rdevp = &p->rdev;
else if (rdev == p->replacement)
--
2.4.6
^ permalink raw reply related
* [PATCH 3/6] raid5-cache: add trim support for log
From: Shaohua Li @ 2015-10-04 16:20 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443973492.git.shli@fb.com>
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);
+ }
+
+ 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
^ permalink raw reply related
* [PATCH 2/6] raid5-cache: move reclaim stop to quiesce
From: Shaohua Li @ 2015-10-04 16:20 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443973492.git.shli@fb.com>
Move reclaim stop to quiesce handling, where is safer for this stuff.
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/raid5-cache.c | 30 ++++++++++++++++++------------
drivers/md/raid5.c | 1 +
drivers/md/raid5.h | 1 +
3 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index a02f9ce..93097a8 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -748,6 +748,24 @@ static void r5l_wake_reclaim(struct r5l_log *log, sector_t space)
md_wakeup_thread(log->reclaim_thread);
}
+void r5l_quiesce(struct r5l_log *log, int state)
+{
+ if (!log || state == 2)
+ return;
+ if (state == 0) {
+ log->reclaim_thread = md_register_thread(r5l_reclaim_thread,
+ log->rdev->mddev, "reclaim");
+ } else if (state == 1) {
+ /*
+ * at this point all stripes are finished, so io_unit is at
+ * least in STRIPE_END state
+ */
+ r5l_wake_reclaim(log, -1L);
+ md_unregister_thread(&log->reclaim_thread);
+ r5l_do_reclaim(log);
+ }
+}
+
struct r5l_recovery_ctx {
struct page *meta_page; /* current meta */
sector_t meta_total_blocks; /* total size of current meta and data */
@@ -1120,19 +1138,7 @@ int r5l_init_log(struct r5conf *conf, struct md_rdev *rdev)
void r5l_exit_log(struct r5l_log *log)
{
- /*
- * at this point all stripes are finished, so io_unit is at least in
- * STRIPE_END state
- */
- r5l_wake_reclaim(log, -1L);
md_unregister_thread(&log->reclaim_thread);
- r5l_do_reclaim(log);
- /*
- * force a super update, r5l_do_reclaim might updated the super.
- * mddev->thread is already stopped
- */
- md_update_sb(log->rdev->mddev, 1);
-
kmem_cache_destroy(log->io_kc);
kfree(log);
}
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a622ccb..216fa3c 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7582,6 +7582,7 @@ static void raid5_quiesce(struct mddev *mddev, int state)
unlock_all_device_hash_locks_irq(conf);
break;
}
+ r5l_quiesce(conf->log, state);
}
static void *raid45_takeover_raid0(struct mddev *mddev, int level)
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 32c8ce8..1ab534c 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -630,4 +630,5 @@ extern void r5l_write_stripe_run(struct r5l_log *log);
extern void r5l_flush_stripe_to_raid(struct r5l_log *log);
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);
#endif
--
2.4.6
^ permalink raw reply related
* [PATCH 1/6] md: show journal for journal disk in disk state sysfs
From: Shaohua Li @ 2015-10-04 16:20 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
In-Reply-To: <cover.1443973492.git.shli@fb.com>
Journal disk state sysfs entry should indicate it's journal
Signed-off-by: Shaohua Li <shli@fb.com>
---
drivers/md/md.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index f1cbb08..c643c9a 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2464,6 +2464,10 @@ state_show(struct md_rdev *rdev, char *page)
len += sprintf(page+len, "%sin_sync",sep);
sep = ",";
}
+ if (test_bit(Journal, &flags)) {
+ len += sprintf(page+len, "%sjournal",sep);
+ sep = ",";
+ }
if (test_bit(WriteMostly, &flags)) {
len += sprintf(page+len, "%swrite_mostly",sep);
sep = ",";
--
2.4.6
^ permalink raw reply related
* [PATCH 0/6] raid5-cache fixes
From: Shaohua Li @ 2015-10-04 16:20 UTC (permalink / raw)
To: linux-raid; +Cc: Kernel-team, songliubraving, hch, dan.j.williams, neilb
Hi,
some fixes for raid5-cache.
patch 1, a small fix
patch 2-3, move reclaim teardown to quiesce handling and add trim support. I
still need md_update_sb there and play the mddev_is_locked trick as the
reconfig_mutex is already hold at md stop. The md_update_sb probably can move
to md core later.
patch 4-6, add error handling. For patch 4, I still need the journal bit check
in in md core, otherwise there is no way to prevent 'echo remove > rdev/state'
to delete journal disk. For patch 6, I didn't change has_failed() yet. Handling
assemble with miss/failed log disk is still on going.
Next step is to make assemble correct with miss/failed log disk. This will need
kernel/utilities cooperation. Song and I are working on it.
Thanks,
Shaohua
Shaohua Li (6):
md: show journal for journal disk in disk state sysfs
raid5-cache: move reclaim stop to quiesce
raid5-cache: add trim support for log
md: don't export log device
md: set In_Sync for log disk
raid5-cache: IO error handling
drivers/md/md.c | 9 ++++--
drivers/md/raid5-cache.c | 83 ++++++++++++++++++++++++++++++++++++++++--------
drivers/md/raid5.c | 7 +++-
drivers/md/raid5.h | 3 ++
4 files changed, 85 insertions(+), 17 deletions(-)
--
2.4.6
^ permalink raw reply
* Re: [PATCH 3/3] raid5-cache: IO error handling
From: Christoph Hellwig @ 2015-10-04 14:44 UTC (permalink / raw)
To: Shaohua Li; +Cc: linux-raid, Kernel-team, songliubraving, dan.j.williams, neilb
In-Reply-To: <e9c915cf18d81f76e122a2533baeb68c56186d8f.1443653794.git.shli@fb.com>
On Wed, Sep 30, 2015 at 04:15:40PM -0700, Shaohua Li wrote:
> 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:
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 430ce5c..8d93af1 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -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;
^ permalink raw reply related
* [GIT PULL REQUEST] md fixes for 4.3
From: Neil Brown @ 2015-10-04 7:39 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jes Sorensen, Julia Lawall, Shaohua Li, linux-raid, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]
Hi Linus,
a few md bug fixes.
Thanks,
NeilBrown
The following changes since commit bcee19f424a0d8c26ecf2607b73c690802658b29:
Merge branch 'for-4.3-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup (2015-09-21 18:26:54 -0700)
are available in the git repository at:
git://neil.brown.name/md tags/md/4.3-fixes
for you to fetch changes up to da6fb7a9e5bd6f04f7e15070f630bdf1ea502841:
md/bitmap: don't pass -1 to bitmap_storage_alloc. (2015-10-02 17:24:13 +1000)
----------------------------------------------------------------
Assorted fixes for md in 4.3-rc
Two tagged for -stable
One is really a cleanup to match and improve kmemcache interface.
----------------------------------------------------------------
Jes Sorensen (1):
md/raid1: Avoid raid1 resync getting stuck
Julia Lawall (1):
md: drop null test before destroy functions
NeilBrown (4):
md: wait for pending superblock updates before switching to read-only
md/raid5: don't index beyond end of array in need_this_block().
md/raid0: apply base queue limits *before* disk_stack_limits
md/bitmap: don't pass -1 to bitmap_storage_alloc.
Shaohua Li (2):
raid5: update analysis state for failed stripe
md: clear CHANGE_PENDING in readonly array
drivers/md/bitmap.c | 3 ++-
drivers/md/md.c | 5 +++++
drivers/md/multipath.c | 3 +--
drivers/md/raid0.c | 12 ++++++------
drivers/md/raid1.c | 11 ++++-------
drivers/md/raid10.c | 9 +++------
drivers/md/raid5.c | 11 +++++++----
7 files changed, 28 insertions(+), 26 deletions(-)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* [md:devel 19/33] ERROR: "wrong_size_cmpxchg" [drivers/md/raid456.ko] undefined!
From: kbuild test robot @ 2015-10-02 20:43 UTC (permalink / raw)
To: Shaohua Li; +Cc: kbuild-all, NeilBrown, linux-raid
[-- Attachment #1: Type: text/plain, Size: 922 bytes --]
tree: git://neil.brown.name/md devel
head: c09760178dc0824490edc6ac4664dbce3c34d002
commit: 07016513b670c6334ad9c24ffa09f2bf50343e60 [19/33] raid5: log reclaim support
config: m68k-sun3_defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 07016513b670c6334ad9c24ffa09f2bf50343e60
# save the attached .config to linux build tree
make.cross ARCH=m68k
Note: the md/devel HEAD c09760178dc0824490edc6ac4664dbce3c34d002 builds fine.
It only hurts bisectibility.
All error/warnings (new ones prefixed by >>):
>> ERROR: "wrong_size_cmpxchg" [drivers/md/raid456.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 10998 bytes --]
^ permalink raw reply
* [md:devel 18/33] mips-linux-gnu-ld: drivers/md/raid5-cache.o: warning: Inconsistent ISA between e_flags and .MIPS.abiflags
From: kbuild test robot @ 2015-10-02 14:04 UTC (permalink / raw)
To: Shaohua Li; +Cc: kbuild-all, NeilBrown, linux-raid
[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]
tree: git://neil.brown.name/md devel
head: c09760178dc0824490edc6ac4664dbce3c34d002
commit: c014ca45166e12c74dd0809911f36ffdb8224450 [18/33] raid5: add basic stripe log
config: mips-ip27_defconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout c014ca45166e12c74dd0809911f36ffdb8224450
# save the attached .config to linux build tree
make.cross ARCH=mips
All warnings (new ones prefixed by >>):
mips-linux-gnu-ld: drivers/md/raid5.o: warning: Inconsistent ISA between e_flags and .MIPS.abiflags
mips-linux-gnu-ld: drivers/md/raid5.o: warning: Inconsistent ISA extensions between e_flags and .MIPS.abiflags
>> mips-linux-gnu-ld: drivers/md/raid5-cache.o: warning: Inconsistent ISA between e_flags and .MIPS.abiflags
>> mips-linux-gnu-ld: drivers/md/raid5-cache.o: warning: Inconsistent ISA extensions between e_flags and .MIPS.abiflags
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 14106 bytes --]
^ permalink raw reply
* [PATCH] dm ioctl: use kvfree() in free_params()
From: Geliang Tang @ 2015-10-02 13:32 UTC (permalink / raw)
To: Alasdair Kergon, Mike Snitzer, dm-devel, Neil Brown
Cc: Geliang Tang, linux-raid, linux-kernel
There is no need to make a flag to tell that this memory is allocated by
kmalloc or vmalloc. Just use kvfree to free the memory.
Signed-off-by: Geliang Tang <geliangtang@163.com>
---
drivers/md/dm-ioctl.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 80a4395..dbb5588 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1669,8 +1669,6 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
return r;
}
-#define DM_PARAMS_KMALLOC 0x0001 /* Params alloced with kmalloc */
-#define DM_PARAMS_VMALLOC 0x0002 /* Params alloced with vmalloc */
#define DM_WIPE_BUFFER 0x0010 /* Wipe input buffer before returning from ioctl */
static void free_params(struct dm_ioctl *param, size_t param_size, int param_flags)
@@ -1678,10 +1676,7 @@ static void free_params(struct dm_ioctl *param, size_t param_size, int param_fla
if (param_flags & DM_WIPE_BUFFER)
memset(param, 0, param_size);
- if (param_flags & DM_PARAMS_KMALLOC)
- kfree(param);
- if (param_flags & DM_PARAMS_VMALLOC)
- vfree(param);
+ kvfree(param);
}
static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kernel,
@@ -1715,8 +1710,6 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
dmi = NULL;
if (param_kernel->data_size <= KMALLOC_MAX_SIZE) {
dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
- if (dmi)
- *param_flags |= DM_PARAMS_KMALLOC;
}
if (!dmi) {
@@ -1724,8 +1717,6 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
noio_flag = memalloc_noio_save();
dmi = __vmalloc(param_kernel->data_size, GFP_NOIO | __GFP_REPEAT | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
memalloc_noio_restore(noio_flag);
- if (dmi)
- *param_flags |= DM_PARAMS_VMALLOC;
}
if (!dmi) {
--
2.5.0
^ permalink raw reply related
* Re: md127 auto created when use "-B" to build a legacy array without superblocks
From: Neil Brown @ 2015-10-02 7:55 UTC (permalink / raw)
To: Yi Zhang; +Cc: linux-raid
In-Reply-To: <597548646.22274436.1441628869039.JavaMail.zimbra@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]
Yi Zhang <yizhan@redhat.com> writes:
> Hi Neil
>
> When testing 00raid1, found the md127 auto created when use "-B" to build a legacy array without superblocks, is it reasonable?
This happens because udev notices a new device has appeared, looks
inside it, sees that it could be part of an md array, and so runs
"mdadm --incremental" on it.
Sometimes we want udev to do that. Sometimes we don't.
There is no easy way for udev to know what we want.
NeilBrown
> pls check below detailed info:
>
> + mdadm -CR /dev/md0 --level=raid1 -n3 /dev/loop0 /dev/loop1 /dev/loop2
> mdadm: /dev/loop0 appears to contain an ext2fs file system
> size=58368K mtime=Thu Jan 1 08:00:00 1970
> mdadm: Note: this array has metadata at the start and
> may not be suitable as a boot device. If you plan to
> store '/boot' on this device please ensure that
> your boot-loader understands md/v1.x metadata, or use
> --metadata=0.90
> mdadm: /dev/loop1 appears to contain an ext2fs file system
> size=38912K mtime=Thu Jan 1 08:00:00 1970
> mdadm: Defaulting to version 1.2 metadata
> mdadm: array /dev/md0 started.
> + mdadm --wait /dev/md0
> + cat /proc/mdstat
> Personalities : [raid6] [raid5] [raid4] [raid1]
> md0 : active raid1 loop2[2] loop1[1] loop0[0]
> 19968 blocks super 1.2 [3/3] [UUU]
>
> unused devices: <none>
> + mdadm -S /dev/md0
> mdadm: stopped /dev/md0
> + mdadm -B /dev/md0 -l raid1 -n2 /dev/loop0 /dev/loop1
> mdadm: array /dev/md0 built and started.
> + sleep 2
> + cat /proc/mdstat
> Personalities : [raid6] [raid5] [raid4] [raid1]
> md127 : inactive md0[0](S)
> 19968 blocks super 1.2
>
> md0 : active raid1 loop1[1] loop0[0]
> 20000 blocks super non-persistent [2/2] [UU]
>
> unused devices: <none>
> [root@dhcp-12-171 bug]# uname -r
> 4.2.0
> [root@dhcp-12-171 bug]# mdadm -D /dev/md0
> /dev/md0:
> Version :
> Creation Time : Mon Sep 7 20:21:20 2015
> Raid Level : raid1
> Array Size : 20000 (19.53 MiB 20.48 MB)
> Used Dev Size : 20000 (19.53 MiB 20.48 MB)
> Raid Devices : 2
> Total Devices : 2
>
> State : clean
> Active Devices : 2
> Working Devices : 2
> Failed Devices : 0
> Spare Devices : 0
>
> Number Major Minor RaidDevice State
> 0 7 0 0 active sync /dev/loop0
> 1 7 1 1 active sync /dev/loop1
> [root@dhcp-12-171 bug]# mdadm -D /dev/md127
> /dev/md127:
> Version : 1.2
> Raid Level : raid0
> Total Devices : 1
> Persistence : Superblock is persistent
>
> State : inactive
>
> Name : dhcp-12-171.nay.redhat.com:0 (local to host dhcp-12-171.nay.redhat.com)
> UUID : 40ace956:a9dd0793:f4984d2b:8431b92b
> Events : 17
>
> Number Major Minor RaidDevice
>
> - 9 0 - /dev/md0
>
> Best Regards,
> Yi Zhang
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* RE: [PATCH 7/8] md: skip resync for raid array with journal
From: Neil Brown @ 2015-10-01 23:50 UTC (permalink / raw)
To: Song Liu, Shaohua Li, linux-raid@vger.kernel.org
Cc: Kernel Team, hch@infradead.org, dan.j.williams@intel.com
In-Reply-To: <C709E4D363AAB64590BFAC54D4C478AA0104B82A61@PRN-MBX02-4.TheFacebook.com>
[-- Attachment #1: Type: text/plain, Size: 5334 bytes --]
Song Liu <songliubraving@fb.com> writes:
>> -----Original Message-----
>> From: Neil Brown [mailto:neilb@suse.de]
>> Sent: Tuesday, September 29, 2015 9:25 PM
>> To: Shaohua Li; linux-raid@vger.kernel.org
>> Cc: Kernel Team; Song Liu; hch@infradead.org; dan.j.williams@intel.com
>> Subject: Re: [PATCH 7/8] md: skip resync for raid array with journal
>>
>> Shaohua Li <shli@fb.com> writes:
>>
>> > If a raid array has journal, the journal can guarantee the
>> > consistency, we can skip resync after a unclean shutdown. The
>> > exception is raid creation or user initiated resync, which we still do a raid
>> resync.
>> >
>> > Signed-off-by: Shaohua Li <shli@fb.com>
>> > ---
>> > drivers/md/md.c | 4 ++++
>> > drivers/md/md.h | 1 +
>> > 2 files changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/md/md.c b/drivers/md/md.c index b3f9eed..95824fb
>> > 100644
>> > --- a/drivers/md/md.c
>> > +++ b/drivers/md/md.c
>> > @@ -1669,6 +1669,8 @@ static int super_1_validate(struct mddev *mddev,
>> struct md_rdev *rdev)
>> > }
>> > set_bit(Journal, &rdev->flags);
>> > rdev->journal_tail = le64_to_cpu(sb->journal_tail);
>> > + if (mddev->recovery_cp == MaxSector)
>> > + set_bit(MD_JOURNAL_CLEAN, &mddev-
>> >flags);
>> > break;
>> > default:
>> > rdev->saved_raid_disk = role;
>> > @@ -1711,6 +1713,8 @@ static void super_1_sync(struct mddev *mddev,
>> struct md_rdev *rdev)
>> > sb->events = cpu_to_le64(mddev->events);
>> > if (mddev->in_sync)
>> > sb->resync_offset = cpu_to_le64(mddev->recovery_cp);
>> > + else if (test_bit(MD_JOURNAL_CLEAN, &mddev->flags))
>> > + sb->resync_offset = cpu_to_le64(MaxSector);
>> > else
>> > sb->resync_offset = cpu_to_le64(0);
>> >
>> > diff --git a/drivers/md/md.h b/drivers/md/md.h index 226f4ba..0288a0b
>> > 100644
>> > --- a/drivers/md/md.h
>> > +++ b/drivers/md/md.h
>> > @@ -236,6 +236,7 @@ struct mddev {
>> > #define MD_STILL_CLOSED 4 /* If set, then array has not been
>> opened since
>> > * md_ioctl checked on it.
>> > */
>> > +#define MD_JOURNAL_CLEAN 5 /* A raid with journal is already clean
>> */
>> >
>> > int suspended;
>> > atomic_t active_io;
>> > --
>> > 1.8.1
>>
>> This looks right as far as it goes, but I don't think it goes far enough.
>> The particular scenario that bothers me is if the array is started without the
>> journal being present.
>> I cannot see anything to prevent that - is there?
>
> I have sent mdadm patches that prevent the array to assemble with missing
> journal, for both --assemble and --incremental:
>
> http://marc.info/?l=linux-raid&m=144080445720123
> http://marc.info/?l=linux-raid&m=144080446120127
>
That is probably good and useful, but the kernel cannot rely on mdadm to
protect it. If the kernel is asked to start a journalled array when no
journal is present, it must do something that is safe.
That might be to refuse, it might be to force a resync, it might be to
somehow "know" if the journal is empty and only force a resync in that
case.
This should be treated much like a "dirty/degraded" start. Currently
the kernel refuses to do that unless a module parameter is set. When
mdadm is asked to --force start a dirty/degraded array, it modifies the
metadata so that it doesn't appear to be degraded. Something similar
might be best for the missing-journal case.
I *think* the current kernel code will allow the array to be started
without a resync even if the journal was not empty, and that is not
safe.
> We can force start in both cases with --force or --run.
>
> Currently, mdadm doesn't clear MD_FEATURE_JOURNAL bit, so we get warning
> every time.
>
> I did miss one step: to mark the missing journal as fault.
>
>>
>> In that case we need to assume that the array is not in-sync, and we need to
>> clear MD_FEATURE_JOURNAL so if it gets stopped and then assembled again
>> with the stale journal doesn't get used.
>>
>> One unfortunate side effect of that is that you couldn't stop the array cleanly
>> (leaving the journal effectively empty) and then restart with no journal and
>> no resync. Is that a problem I wonder?
>>
>> I'm not sure what the best solution is here, but we need a clear
>> understanding of what happens if you try to assemble an array without the
>> journal where previously it had one, and I don't think the current code gets it
>> right.
>
> I just talked with Shaohua. We think we can probably do the following:
>
> 1. When assemble with cache device missing, show the warning (with --force
> option). Run resync if previous shutdown is not clean (this is the default
> behavior.
Probably sensible. But I think the current code never marks the array as
"not clean" when there is a journal.
>
> 2. When we get I/O error on the journal device at run time, make the whole
> array as read only.
Yes, I think that is best.
>
> 3. Add new operation that adds new journal device to an array with missing
> journal. We can limit this option to inactive array only.
It would be great if we could add a journal to an active array...
>
> Would this follow cover all the cases?
Quite possibly. I don't want to commit myself until I see the code :-)
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Jan Kara @ 2015-10-01 23:07 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Mike Snitzer, Seth Forshee, Kent Overstreet, Alasdair Kergon,
dm-devel, Neil Brown, David Woodhouse, Brian Norris,
Alexander Viro, Jan Kara, Jeff Layton, J. Bruce Fields,
Serge Hallyn, Andy Lutomirski, linux-fsdevel,
linux-security-module, selinux, linux-kernel, linux-mtd,
linux-bcache, linux-raid
In-Reply-To: <87wpv6a8hl.fsf@x220.int.ebiederm.org>
On Thu 01-10-15 10:55:50, Eric W. Biederman wrote:
> The goal if possible is to run things like docker without needed to be
> root or even more fun to run docker in a container, and in general
> enable nested containers.
Frankly at the filesystem side we are rather far from being able to safely
mount untrusted device and I don't think we'll ever be robust enough to
tolerate e.g. user changing the disk while fs is using it. So would this be
FUSE-only thing or is someone still hoping that general purpose filesystems
will be robust enough in future?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [PATCH] crash in md-raid1 and md-raid10 due to incorrect list manipulation
From: Mikulas Patocka @ 2015-10-01 19:17 UTC (permalink / raw)
To: NeilBrown; +Cc: linux-raid, dm-devel, linux-kernel, Mike Snitzer
The commit 55ce74d4bfe1b9444436264c637f39a152d1e5ac (md/raid1: ensure
device failure recorded before write request returns) is causing crash in
the LVM2 testsuite test shell/lvchange-raid.sh. For me the crash is 100%
reproducible.
The reason for the crash is that the newly added code in raid1d moves the
list from conf->bio_end_io_list to tmp, then tests if tmp is non-empty and
then incorrectly pops the bio from conf->bio_end_io_list (which is empty
because the list was alrady moved).
Raid-10 has a similar bug.
Kernel Fault: Code=15 regs=000000006ccb8640 (Addr=0000000100000000)
CPU: 3 PID: 1930 Comm: mdX_raid1 Not tainted 4.2.0-rc5-bisect+ #35
task: 000000006cc1f258 ti: 000000006ccb8000 task.ti: 000000006ccb8000
YZrvWESTHLNXBCVMcbcbcbcbOGFRQPDI
PSW: 00001000000001001111111000001111 Not tainted
r00-03 000000ff0804fe0f 000000001059d000 000000001059f818 000000007f16be38
r04-07 000000001059d000 000000007f16be08 0000000000200200 0000000000000001
r08-11 000000006ccb8260 000000007b7934d0 0000000000000001 0000000000000000
r12-15 000000004056f320 0000000000000000 0000000000013dd0 0000000000000000
r16-19 00000000f0d00ae0 0000000000000000 0000000000000000 0000000000000001
r20-23 000000000800000f 0000000042200390 0000000000000000 0000000000000000
r24-27 0000000000000001 000000000800000f 000000007f16be08 000000001059d000
r28-31 0000000100000000 000000006ccb8560 000000006ccb8640 0000000000000000
sr00-03 0000000000249800 0000000000000000 0000000000000000 0000000000249800
sr04-07 0000000000000000 0000000000000000 0000000000000000 0000000000000000
IASQ: 0000000000000000 0000000000000000 IAOQ: 000000001059f61c 000000001059f620
IIR: 0f8010c6 ISR: 0000000000000000 IOR: 0000000100000000
CPU: 3 CR30: 000000006ccb8000 CR31: 0000000000000000
ORIG_R28: 000000001059d000
IAOQ[0]: call_bio_endio+0x34/0x1a8 [raid1]
IAOQ[1]: call_bio_endio+0x38/0x1a8 [raid1]
RP(r2): raid_end_bio_io+0x88/0x168 [raid1]
Backtrace:
[<000000001059f818>] raid_end_bio_io+0x88/0x168 [raid1]
[<00000000105a4f64>] raid1d+0x144/0x1640 [raid1]
[<000000004017fd5c>] kthread+0x144/0x160
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Fixes: 55ce74d4bfe1 ("md/raid1: ensure device failure recorded before write request returns.")
Fixes: 95af587e95aa ("md/raid10: ensure device failure recorded before write request returns.")
---
drivers/md/raid1.c | 4 ++--
drivers/md/raid10.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
Index: linux-2.6/drivers/md/raid1.c
===================================================================
--- linux-2.6.orig/drivers/md/raid1.c 2015-10-01 21:10:05.000000000 +0200
+++ linux-2.6/drivers/md/raid1.c 2015-10-01 21:10:58.000000000 +0200
@@ -2437,8 +2437,8 @@ static void raid1d(struct md_thread *thr
}
spin_unlock_irqrestore(&conf->device_lock, flags);
while (!list_empty(&tmp)) {
- r1_bio = list_first_entry(&conf->bio_end_io_list,
- struct r1bio, retry_list);
+ r1_bio = list_first_entry(&tmp, struct r1bio,
+ retry_list);
list_del(&r1_bio->retry_list);
raid_end_bio_io(r1_bio);
}
Index: linux-2.6/drivers/md/raid10.c
===================================================================
--- linux-2.6.orig/drivers/md/raid10.c 2015-10-01 21:11:02.000000000 +0200
+++ linux-2.6/drivers/md/raid10.c 2015-10-01 21:11:19.000000000 +0200
@@ -2804,8 +2804,8 @@ static void raid10d(struct md_thread *th
}
spin_unlock_irqrestore(&conf->device_lock, flags);
while (!list_empty(&tmp)) {
- r10_bio = list_first_entry(&conf->bio_end_io_list,
- struct r10bio, retry_list);
+ r10_bio = list_first_entry(&tmp, struct r10bio,
+ retry_list);
list_del(&r10_bio->retry_list);
raid_end_bio_io(r10_bio);
}
^ permalink raw reply
* mdraid + dm-multipath - non-dm-mulitpath boot md coexisting with dm-multi md data volume?
From: Jeff Johnson @ 2015-10-01 18:55 UTC (permalink / raw)
To: linux-raid
Greetings,
I'm looking for advice on configuring mdadm.conf to allow for the
coexisting of a non dm-multipath boot/root md device and a dm-multipath
configured md device data volume.
The issue I am running into is that on boot mdraid assumes control of
all disks with md superblocks on them before multipath starts. This
appears to be due to the boot/root being md devices, multipath never
gets to own the other disks in the system and then assemble on mpath
devices in /dev/disk/by-id.
I have tried using DEVICE declarations in mdadm.conf but so far the
result is either the boot volumes don't get recognized (assemble +
switchroot fails) but the data volume gets assembled with dm-multipath
devices, or the boot/root md devices assemble and the dm-multipath based
md devices get assembled as sd member devices and dm-multipath is disabled.
Some of the DEVICE declarations I tried in mdadm.conf are below. Two on
the same line or two separate lines. The non dm-mulitpath boot devices
are Intel SATA SSDs on the onboard SATA ports. The dm-mulitpath RAID-10
data volume is on an external SAS HBA with valid multipath
configuration. I have remade the initramfs after these changes (dracut -f).
DEVICE /dev/disk/by-id/scsi-SATA_INTEL_SSD.*-.*
/dev/disk/by-id/dm-uuid-.*-mpath-.*
and
DEVICE /dev/disk/by-id/scsi-SATA_INTEL_SSD.*-.*
DEVICE /dev/disk/by-id/dm-uuid-.*-mpath-.*
These examples find the SAS attached dm-multipath devices fine and
assemble the mdraid volumes but the boot devices are not found or
assembled. A look via rdshell shows the
/dev/disk/by-id/scsi-SATA_INTEL_SSD.*-.* devices listed in the
/dev/disk/by-id directory. It appears mdadm doesn't like the declaration
for the SSDs and won't find them. I haven't been able to figure out why
so far.
Current state (/proc/mdstat)
cat /proc/mdstat
Personalities : [raid1] [raid10]
md4 : active raid10 dm-11[3] dm-5[2] dm-7[1] dm-9[0]
584615936 blocks super 1.2 512K chunks 2 near-copies [4/4] [UUUU]
bitmap: 0/5 pages [0KB], 65536KB chunk
md3 : active raid1 dm-10[3] dm-2[2] dm-4[1] dm-8[0]
521792 blocks super 1.2 [4/4] [UUUU]
md0 : active raid1 sdb1[1] sda1[0]
255936 blocks super 1.0 [2/2] [UU]
md2 : active raid1 sdb3[1] sda3[0]
225653760 blocks super 1.1 [2/2] [UU]
bitmap: 0/2 pages [0KB], 65536KB chunk
md1 : active raid1 sda2[0] sdb2[1]
8380416 blocks super 1.1 [2/2] [UU]
unused devices: <none>
OS: CentOS 6.6 x86_64 2.6.32-504.el6.x86_64
If anyone has suggestions of how to configure a proper set of DEVICE
declarations to assemble the SATA connected member drives of the
boot/root volumes without relying on the default "DEVICE partitions
containers" where the multipath devices get assembled as sd devices
prior to multipath starting I would appreciate the advice.
Thanks,
--Jeff
^ permalink raw reply
* RE: [PATCH 7/8] md: skip resync for raid array with journal
From: Song Liu @ 2015-10-01 17:47 UTC (permalink / raw)
To: Neil Brown, Shaohua Li, linux-raid@vger.kernel.org
Cc: Kernel Team, hch@infradead.org, dan.j.williams@intel.com
In-Reply-To: <871tdgmt4w.fsf@notabene.neil.brown.name>
> -----Original Message-----
> From: Neil Brown [mailto:neilb@suse.de]
> Sent: Tuesday, September 29, 2015 9:25 PM
> To: Shaohua Li; linux-raid@vger.kernel.org
> Cc: Kernel Team; Song Liu; hch@infradead.org; dan.j.williams@intel.com
> Subject: Re: [PATCH 7/8] md: skip resync for raid array with journal
>
> Shaohua Li <shli@fb.com> writes:
>
> > If a raid array has journal, the journal can guarantee the
> > consistency, we can skip resync after a unclean shutdown. The
> > exception is raid creation or user initiated resync, which we still do a raid
> resync.
> >
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> > drivers/md/md.c | 4 ++++
> > drivers/md/md.h | 1 +
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c index b3f9eed..95824fb
> > 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -1669,6 +1669,8 @@ static int super_1_validate(struct mddev *mddev,
> struct md_rdev *rdev)
> > }
> > set_bit(Journal, &rdev->flags);
> > rdev->journal_tail = le64_to_cpu(sb->journal_tail);
> > + if (mddev->recovery_cp == MaxSector)
> > + set_bit(MD_JOURNAL_CLEAN, &mddev-
> >flags);
> > break;
> > default:
> > rdev->saved_raid_disk = role;
> > @@ -1711,6 +1713,8 @@ static void super_1_sync(struct mddev *mddev,
> struct md_rdev *rdev)
> > sb->events = cpu_to_le64(mddev->events);
> > if (mddev->in_sync)
> > sb->resync_offset = cpu_to_le64(mddev->recovery_cp);
> > + else if (test_bit(MD_JOURNAL_CLEAN, &mddev->flags))
> > + sb->resync_offset = cpu_to_le64(MaxSector);
> > else
> > sb->resync_offset = cpu_to_le64(0);
> >
> > diff --git a/drivers/md/md.h b/drivers/md/md.h index 226f4ba..0288a0b
> > 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -236,6 +236,7 @@ struct mddev {
> > #define MD_STILL_CLOSED 4 /* If set, then array has not been
> opened since
> > * md_ioctl checked on it.
> > */
> > +#define MD_JOURNAL_CLEAN 5 /* A raid with journal is already clean
> */
> >
> > int suspended;
> > atomic_t active_io;
> > --
> > 1.8.1
>
> This looks right as far as it goes, but I don't think it goes far enough.
> The particular scenario that bothers me is if the array is started without the
> journal being present.
> I cannot see anything to prevent that - is there?
I have sent mdadm patches that prevent the array to assemble with missing
journal, for both --assemble and --incremental:
http://marc.info/?l=linux-raid&m=144080445720123
http://marc.info/?l=linux-raid&m=144080446120127
We can force start in both cases with --force or --run.
Currently, mdadm doesn't clear MD_FEATURE_JOURNAL bit, so we get warning
every time.
I did miss one step: to mark the missing journal as fault.
>
> In that case we need to assume that the array is not in-sync, and we need to
> clear MD_FEATURE_JOURNAL so if it gets stopped and then assembled again
> with the stale journal doesn't get used.
>
> One unfortunate side effect of that is that you couldn't stop the array cleanly
> (leaving the journal effectively empty) and then restart with no journal and
> no resync. Is that a problem I wonder?
>
> I'm not sure what the best solution is here, but we need a clear
> understanding of what happens if you try to assemble an array without the
> journal where previously it had one, and I don't think the current code gets it
> right.
I just talked with Shaohua. We think we can probably do the following:
1. When assemble with cache device missing, show the warning (with --force
option). Run resync if previous shutdown is not clean (this is the default
behavior.
2. When we get I/O error on the journal device at run time, make the whole
array as read only.
3. Add new operation that adds new journal device to an array with missing
journal. We can limit this option to inactive array only.
Would this follow cover all the cases?
Thanks,
Song
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Eric W. Biederman @ 2015-10-01 15:55 UTC (permalink / raw)
To: Mike Snitzer
Cc: Seth Forshee, Kent Overstreet, Alasdair Kergon, dm-devel,
Neil Brown, David Woodhouse, Brian Norris, Alexander Viro,
Jan Kara, Jeff Layton, J. Bruce Fields, Serge Hallyn,
Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, linux-raid
In-Reply-To: <20151001134052.GB27818@redhat.com>
Mike Snitzer <snitzer@redhat.com> writes:
> What layer establishes access rights to historically root-only
> priviledged block devices? Is it user namespaces?
Block devices are weird.
Mounts historically have not checked the permissions on the block
devices because a mounter has CAP_SYS_ADMIN.
Unprivileged users are allowes to read/write block devices if
someone has given them permissions on the device node in the
filesystem.
The thinking with this patchset is to start performing the normal
block device access permission checks when mounting filesystems
when the mounter does not have the global CAP_SYS_ADMIN permission.
The truth is we are not much past the point of realizing that there were
no permission checks to use the actual block device passed in to mount,
so we could still be missing something. There is a lot going on with dm,
md, and lvm. I don't know if the model of just look at the block device
inode and perform the permission checks is good enough.
> I haven't kept up with user namespaces as it relates to stacking block
> drivers like DM. But I'm happy to come up to speed and at the same time
> help you verify all works as expected with DM blocks devices...
We are just getting there. But if you can help that would be great.
The primary concern with dm is what happens when unprivileged users get
ahold of the code, and what happens when evil users corrupt the on-disk
format.
In principle dm like loop should be safe to use if there are not bugs
that make it unsafe for unprivileged users to access the code.
The goal if possible is to run things like docker without needed to be
root or even more fun to run docker in a container, and in general
enable nested containers.
Eric
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Seth Forshee @ 2015-10-01 15:55 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Kent Overstreet, Alasdair Kergon, Mike Snitzer, dm-devel,
Neil Brown, David Woodhouse, Brian Norris, Alexander Viro,
Jan Kara, Jeff Layton, J. Bruce Fields, Serge Hallyn,
Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, linux-raid
In-Reply-To: <87mvw2d2cn.fsf@x220.int.ebiederm.org>
On Thu, Oct 01, 2015 at 10:40:08AM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
>
> > When mounting a filesystem on a block device there is currently
> > no verification that the user has appropriate access to the
> > device file passed to mount. This has not been an issue so far
> > since the user in question has always been root, but this must
> > be changed before allowing unprivileged users to mount in user
> > namespaces.
> >
> > To fix this, add an argument to lookup_bdev() to specify the
> > required permissions. If the mask of permissions is zero, or
> > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > otherwise the lookup fails if the user does not have the
> > specified access rights for the inode at the supplied path.
> >
> > Callers associated with mounting are updated to pass permission
> > masks to lookup_bdev() so that these mounts will fail for an
> > unprivileged user who lacks permissions for the block device
> > inode. All other callers pass 0 to maintain their current
> > behaviors.
> >
>
> Seth can you split this patch?
>
> One patch to add an argument to lookup_bdev,
> and then for each kind of callsite a follow-on patch (if we are ready
> for that).
>
> That will separate the logical changes and make things easier to track
> via bisect and more importantly easier to review things.
Sure, I'll do that.
Seth
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Eric W. Biederman @ 2015-10-01 15:40 UTC (permalink / raw)
To: Seth Forshee
Cc: Kent Overstreet, Alasdair Kergon, Mike Snitzer, dm-devel,
Neil Brown, David Woodhouse, Brian Norris, Alexander Viro,
Jan Kara, Jeff Layton, J. Bruce Fields, Serge Hallyn,
Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, linux-raid
In-Reply-To: <1443644116-41366-2-git-send-email-seth.forshee@canonical.com>
Seth Forshee <seth.forshee@canonical.com> writes:
> When mounting a filesystem on a block device there is currently
> no verification that the user has appropriate access to the
> device file passed to mount. This has not been an issue so far
> since the user in question has always been root, but this must
> be changed before allowing unprivileged users to mount in user
> namespaces.
>
> To fix this, add an argument to lookup_bdev() to specify the
> required permissions. If the mask of permissions is zero, or
> if the user has CAP_SYS_ADMIN, the permission check is skipped,
> otherwise the lookup fails if the user does not have the
> specified access rights for the inode at the supplied path.
>
> Callers associated with mounting are updated to pass permission
> masks to lookup_bdev() so that these mounts will fail for an
> unprivileged user who lacks permissions for the block device
> inode. All other callers pass 0 to maintain their current
> behaviors.
>
Seth can you split this patch?
One patch to add an argument to lookup_bdev,
and then for each kind of callsite a follow-on patch (if we are ready
for that).
That will separate the logical changes and make things easier to track
via bisect and more importantly easier to review things.
Eric
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
> drivers/md/bcache/super.c | 2 +-
> drivers/md/dm-table.c | 2 +-
> drivers/mtd/mtdsuper.c | 6 +++++-
> fs/block_dev.c | 18 +++++++++++++++---
> fs/quota/quota.c | 2 +-
> include/linux/fs.h | 2 +-
> 6 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 679a093a3bf6..e8287b0d1dac 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1926,7 +1926,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> sb);
> if (IS_ERR(bdev)) {
> if (bdev == ERR_PTR(-EBUSY)) {
> - bdev = lookup_bdev(strim(path));
> + bdev = lookup_bdev(strim(path), 0);
> mutex_lock(&bch_register_lock);
> if (!IS_ERR(bdev) && bch_is_open(bdev))
> err = "device already registered";
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index e76ed003769e..35bb3ea4cbe2 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> BUG_ON(!t);
>
> /* convert the path to a device */
> - bdev = lookup_bdev(path);
> + bdev = lookup_bdev(path, 0);
> if (IS_ERR(bdev)) {
> dev = name_to_dev_t(path);
> if (!dev)
> diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
> index 20c02a3b7417..5d7e7705fed8 100644
> --- a/drivers/mtd/mtdsuper.c
> +++ b/drivers/mtd/mtdsuper.c
> @@ -125,6 +125,7 @@ struct dentry *mount_mtd(struct file_system_type *fs_type, int flags,
> #ifdef CONFIG_BLOCK
> struct block_device *bdev;
> int ret, major;
> + int perm;
> #endif
> int mtdnr;
>
> @@ -176,7 +177,10 @@ struct dentry *mount_mtd(struct file_system_type *fs_type, int flags,
> /* try the old way - the hack where we allowed users to mount
> * /dev/mtdblock$(n) but didn't actually _use_ the blockdev
> */
> - bdev = lookup_bdev(dev_name);
> + perm = MAY_READ;
> + if (!(flags & MS_RDONLY))
> + perm |= MAY_WRITE;
> + bdev = lookup_bdev(dev_name, perm);
> if (IS_ERR(bdev)) {
> ret = PTR_ERR(bdev);
> pr_debug("MTDSB: lookup_bdev() returned %d\n", ret);
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 26cee058dc02..54d94cd64577 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1394,9 +1394,14 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
> void *holder)
> {
> struct block_device *bdev;
> + int perm = 0;
> int err;
>
> - bdev = lookup_bdev(path);
> + if (mode & FMODE_READ)
> + perm |= MAY_READ;
> + if (mode & FMODE_WRITE)
> + perm |= MAY_WRITE;
> + bdev = lookup_bdev(path, perm);
> if (IS_ERR(bdev))
> return bdev;
>
> @@ -1706,12 +1711,14 @@ EXPORT_SYMBOL(ioctl_by_bdev);
> /**
> * lookup_bdev - lookup a struct block_device by name
> * @pathname: special file representing the block device
> + * @mask: rights to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
> *
> * Get a reference to the blockdevice at @pathname in the current
> * namespace if possible and return it. Return ERR_PTR(error)
> - * otherwise.
> + * otherwise. If @mask is non-zero, check for access rights to the
> + * inode at @pathname.
> */
> -struct block_device *lookup_bdev(const char *pathname)
> +struct block_device *lookup_bdev(const char *pathname, int mask)
> {
> struct block_device *bdev;
> struct inode *inode;
> @@ -1726,6 +1733,11 @@ struct block_device *lookup_bdev(const char *pathname)
> return ERR_PTR(error);
>
> inode = d_backing_inode(path.dentry);
> + if (mask != 0 && !capable(CAP_SYS_ADMIN)) {
> + error = __inode_permission(inode, mask);
> + if (error)
> + goto fail;
> + }
> error = -ENOTBLK;
> if (!S_ISBLK(inode->i_mode))
> goto fail;
> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> index 3746367098fd..a40eaecbd5cc 100644
> --- a/fs/quota/quota.c
> +++ b/fs/quota/quota.c
> @@ -733,7 +733,7 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
>
> if (IS_ERR(tmp))
> return ERR_CAST(tmp);
> - bdev = lookup_bdev(tmp->name);
> + bdev = lookup_bdev(tmp->name, 0);
> putname(tmp);
> if (IS_ERR(bdev))
> return ERR_CAST(bdev);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 458ee7b213be..cc18dfb0b98e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2388,7 +2388,7 @@ static inline void unregister_chrdev(unsigned int major, const char *name)
> #define BLKDEV_MAJOR_HASH_SIZE 255
> extern const char *__bdevname(dev_t, char *buffer);
> extern const char *bdevname(struct block_device *bdev, char *buffer);
> -extern struct block_device *lookup_bdev(const char *);
> +extern struct block_device *lookup_bdev(const char *, int mask);
> extern void blkdev_show(struct seq_file *,off_t);
>
> #else
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Seth Forshee @ 2015-10-01 14:41 UTC (permalink / raw)
To: Mike Snitzer
Cc: Eric W. Biederman, Kent Overstreet, Alasdair Kergon, dm-devel,
Neil Brown, David Woodhouse, Brian Norris, Alexander Viro,
Jan Kara, Jeff Layton, J. Bruce Fields, Serge Hallyn,
Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, linux-raid
In-Reply-To: <20151001134052.GB27818@redhat.com>
On Thu, Oct 01, 2015 at 09:40:52AM -0400, Mike Snitzer wrote:
> On Thu, Oct 01 2015 at 8:55am -0400,
> Seth Forshee <seth.forshee@canonical.com> wrote:
>
> > On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> > > On Wed, Sep 30 2015 at 4:15pm -0400,
> > > Seth Forshee <seth.forshee@canonical.com> wrote:
> > >
> > > > When mounting a filesystem on a block device there is currently
> > > > no verification that the user has appropriate access to the
> > > > device file passed to mount. This has not been an issue so far
> > > > since the user in question has always been root, but this must
> > > > be changed before allowing unprivileged users to mount in user
> > > > namespaces.
> > > >
> > > > To fix this, add an argument to lookup_bdev() to specify the
> > > > required permissions. If the mask of permissions is zero, or
> > > > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > > > otherwise the lookup fails if the user does not have the
> > > > specified access rights for the inode at the supplied path.
> > > >
> > > > Callers associated with mounting are updated to pass permission
> > > > masks to lookup_bdev() so that these mounts will fail for an
> > > > unprivileged user who lacks permissions for the block device
> > > > inode. All other callers pass 0 to maintain their current
> > > > behaviors.
> > > >
> > > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > > ---
> > > > drivers/md/bcache/super.c | 2 +-
> > > > drivers/md/dm-table.c | 2 +-
> > > > drivers/mtd/mtdsuper.c | 6 +++++-
> > > > fs/block_dev.c | 18 +++++++++++++++---
> > > > fs/quota/quota.c | 2 +-
> > > > include/linux/fs.h | 2 +-
> > > > 6 files changed, 24 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > > index e76ed003769e..35bb3ea4cbe2 100644
> > > > --- a/drivers/md/dm-table.c
> > > > +++ b/drivers/md/dm-table.c
> > > > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > BUG_ON(!t);
> > > >
> > > > /* convert the path to a device */
> > > > - bdev = lookup_bdev(path);
> > > > + bdev = lookup_bdev(path, 0);
> > > > if (IS_ERR(bdev)) {
> > > > dev = name_to_dev_t(path);
> > > > if (!dev)
> > >
> > > Given dm_get_device() is passed @mode why not have it do something like
> > > you did in blkdev_get_by_path()? e.g.:
> >
> > I only dealt with code related to mounting in this patch since that's
> > what I'm working on. I have it on my TODO list to consider converting
> > other callers of lookup_bdev. But if you're sure doing so makes sense
> > for dm_get_device and that it won't cause regressions then I could add a
> > patch for it.
>
> OK, dm_get_device() is called in DM device activation path (by tools
> like lvm2).
>
> After lookup_bdev() it goes on to call blkdev_get_by_dev() with this
> call chain:
> dm_get_device -> dm_get_table_device -> open_table_device -> blkdev_get_by_dev
>
> Not immediately clear to me why we'd need to augment blkdev_get_by_dev()
> to do this checking also.
>
> However, thinking further: In a device stack (e.g. dm/lvm2, md, etc)
> new virtual block devices are created that layer ontop of the
> traditional block devices. This level of indirection may cause your
> lookup_bdev() check to go on to succeed (if access constraints were not
> established on the upper level dm or md device?). I'm just thinking
> outloud here: but have you verified your changes work as intended on
> devices created with either lvm2 or mdadm?
>
> What layer establishes access rights to historically root-only
> priviledged block devices? Is it user namespaces?
I'm going to start with this last question and work my way backwards.
Who determines access rights to the block devices varies to some degree.
Any normal user could unshare the user/mount namespaces and still see
all the block devices in /dev. If we're going to allow that user to then
mount block devices in their private namespaces, the kernel must verify
that the user has appropriate permissions for the block device inode.
That's the point of this patch. In a container context the host process
which sets up the container might share some block devices into the
container via bind mounts, in which case it would be responsible for
setting up access rights (both via inode permissions and potentially via
devcgroups). I also have some patches I'm working on for a loop psuedo
filesystem which would allow a container to allocate loop devices for
its own use (via the loop-control device), in which case the kernel
creates a device node in the loopfs filesystem from which the new loop
device was allocated, owned by the root user in the user namespace
associated with the loopfs superblock.
Obviously a DM block device is more complicated than a traditional block
device. This patch should ensure that if an unprivileged user tries to
mount a DM blkdev that it fails if the user lacks permissions for the DM
blkdev inode. There's also the blkdevs behind the DM blkdev, and I'm not
sure whether we need to additionally verify that the user has access to
those devices. Maybe it's enough that someone with access to them set up
the DM device, and someone with sufficient privileges gave that user
access to the DM device. Do you have any thoughts?
As for activating a DM device, I assume only root is permitted to do
this. In that case lookup_bdev will skip the inode permission check
even if a non-zero permission mask is passed.
> I haven't kept up with user namespaces as it relates to stacking block
> drivers like DM. But I'm happy to come up to speed and at the same time
> help you verify all works as expected with DM blocks devices...
That's great, I really appreciate it.
Seth
^ permalink raw reply
* Re: [PATCH 1/5] fs: Verify access of user towards block device file when mounting
From: Mike Snitzer @ 2015-10-01 13:40 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Kent Overstreet, Alasdair Kergon, dm-devel,
Neil Brown, David Woodhouse, Brian Norris, Alexander Viro,
Jan Kara, Jeff Layton, J. Bruce Fields, Serge Hallyn,
Andy Lutomirski, linux-fsdevel, linux-security-module, selinux,
linux-kernel, linux-mtd, linux-bcache, linux-raid
In-Reply-To: <20151001125508.GA101875@ubuntu-hedt>
On Thu, Oct 01 2015 at 8:55am -0400,
Seth Forshee <seth.forshee@canonical.com> wrote:
> On Wed, Sep 30, 2015 at 07:42:15PM -0400, Mike Snitzer wrote:
> > On Wed, Sep 30 2015 at 4:15pm -0400,
> > Seth Forshee <seth.forshee@canonical.com> wrote:
> >
> > > When mounting a filesystem on a block device there is currently
> > > no verification that the user has appropriate access to the
> > > device file passed to mount. This has not been an issue so far
> > > since the user in question has always been root, but this must
> > > be changed before allowing unprivileged users to mount in user
> > > namespaces.
> > >
> > > To fix this, add an argument to lookup_bdev() to specify the
> > > required permissions. If the mask of permissions is zero, or
> > > if the user has CAP_SYS_ADMIN, the permission check is skipped,
> > > otherwise the lookup fails if the user does not have the
> > > specified access rights for the inode at the supplied path.
> > >
> > > Callers associated with mounting are updated to pass permission
> > > masks to lookup_bdev() so that these mounts will fail for an
> > > unprivileged user who lacks permissions for the block device
> > > inode. All other callers pass 0 to maintain their current
> > > behaviors.
> > >
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > ---
> > > drivers/md/bcache/super.c | 2 +-
> > > drivers/md/dm-table.c | 2 +-
> > > drivers/mtd/mtdsuper.c | 6 +++++-
> > > fs/block_dev.c | 18 +++++++++++++++---
> > > fs/quota/quota.c | 2 +-
> > > include/linux/fs.h | 2 +-
> > > 6 files changed, 24 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > index e76ed003769e..35bb3ea4cbe2 100644
> > > --- a/drivers/md/dm-table.c
> > > +++ b/drivers/md/dm-table.c
> > > @@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > BUG_ON(!t);
> > >
> > > /* convert the path to a device */
> > > - bdev = lookup_bdev(path);
> > > + bdev = lookup_bdev(path, 0);
> > > if (IS_ERR(bdev)) {
> > > dev = name_to_dev_t(path);
> > > if (!dev)
> >
> > Given dm_get_device() is passed @mode why not have it do something like
> > you did in blkdev_get_by_path()? e.g.:
>
> I only dealt with code related to mounting in this patch since that's
> what I'm working on. I have it on my TODO list to consider converting
> other callers of lookup_bdev. But if you're sure doing so makes sense
> for dm_get_device and that it won't cause regressions then I could add a
> patch for it.
OK, dm_get_device() is called in DM device activation path (by tools
like lvm2).
After lookup_bdev() it goes on to call blkdev_get_by_dev() with this
call chain:
dm_get_device -> dm_get_table_device -> open_table_device -> blkdev_get_by_dev
Not immediately clear to me why we'd need to augment blkdev_get_by_dev()
to do this checking also.
However, thinking further: In a device stack (e.g. dm/lvm2, md, etc)
new virtual block devices are created that layer ontop of the
traditional block devices. This level of indirection may cause your
lookup_bdev() check to go on to succeed (if access constraints were not
established on the upper level dm or md device?). I'm just thinking
outloud here: but have you verified your changes work as intended on
devices created with either lvm2 or mdadm?
What layer establishes access rights to historically root-only
priviledged block devices? Is it user namespaces?
I haven't kept up with user namespaces as it relates to stacking block
drivers like DM. But I'm happy to come up to speed and at the same time
help you verify all works as expected with DM blocks devices...
Mike
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox