* [PATCH v2 0/6] BTT error clearing rework
@ 2017-07-17 21:27 Vishal Verma
2017-07-17 21:27 ` [PATCH v2 1/6] btt: fix a missed NVDIMM_IO_ATOMIC case in the write path Vishal Verma
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Vishal Verma @ 2017-07-17 21:27 UTC (permalink / raw)
To: linux-nvdimm; +Cc: Rafael J. Wysocki
Changes in v2:
- Drop the ACPI allocation change patch. Instead use
memalloc_noio_{save,restore} to set the GFP_NOIO flag around anything
that can be expected to call into ACPI for clearing errors. (Rafael, Dan).
Clearing errors or badblocks during a BTT write requires sending an ACPI
DSM, which means potentially sleeping. Since a BTT IO happens in atomic
context (preemption disabled, spinlocks may be held), we cannot perform
error clearing in the course of an IO. Due to this error clearing for
BTT IOs has hitherto been disabled.
This series fixes these problems by moving the error clearing out of
the atomic sections in the BTT.
Also fix a potential deadlock that can occur while clearing errors
from either BTT or pmem due to memory allocations in the IO path.
Vishal Verma (6):
btt: fix a missed NVDIMM_IO_ATOMIC case in the write path
btt: refactor map entry operations with macros
btt: ensure that flags were also unchanged during a map_read
btt: cache sector_size in arena_info
libnvdimm, btt: rework error clearing
pmem, btt: fix potential deadlock while clearing errors
drivers/nvdimm/btt.c | 117 ++++++++++++++++++++++++++++++++++++++++++-------
drivers/nvdimm/btt.h | 11 +++++
drivers/nvdimm/claim.c | 9 +---
drivers/nvdimm/pmem.c | 4 ++
4 files changed, 116 insertions(+), 25 deletions(-)
--
2.9.3
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2 1/6] btt: fix a missed NVDIMM_IO_ATOMIC case in the write path 2017-07-17 21:27 [PATCH v2 0/6] BTT error clearing rework Vishal Verma @ 2017-07-17 21:27 ` Vishal Verma 2017-07-17 21:27 ` [PATCH v2 2/6] btt: refactor map entry operations with macros Vishal Verma ` (4 subsequent siblings) 5 siblings, 0 replies; 11+ messages in thread From: Vishal Verma @ 2017-07-17 21:27 UTC (permalink / raw) To: linux-nvdimm; +Cc: Rafael J. Wysocki The IO context conversion for rw_bytes missed a case in the BTT write path (btt_map_write) which should've been marked as atomic. In reality this should not cause a problem, because map writes are to small for nsio_rw_bytes to attempt error clearing, but it should be fixed for posterity. Add a might_sleep() in the non-atomic section of nsio_rw_bytes so that things like the nfit unit tests, which don't actually sleep, can catch bugs like this. Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/nvdimm/btt.c | 3 ++- drivers/nvdimm/claim.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 2af329d..52aa96f 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -1155,7 +1155,8 @@ static int btt_write_pg(struct btt *btt, struct bio_integrity_payload *bip, if (ret) goto out_map; - ret = btt_map_write(arena, premap, new_postmap, 0, 0, 0); + ret = btt_map_write(arena, premap, new_postmap, 0, 0, + NVDIMM_IO_ATOMIC); if (ret) goto out_map; diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c index f8ad92b..8f29937 100644 --- a/drivers/nvdimm/claim.c +++ b/drivers/nvdimm/claim.c @@ -292,6 +292,7 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns, && !(flags & NVDIMM_IO_ATOMIC)) { long cleared; + might_sleep(); cleared = nvdimm_clear_poison(&ndns->dev, nsio->res.start + offset, size); if (cleared < size) -- 2.9.3 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/6] btt: refactor map entry operations with macros 2017-07-17 21:27 [PATCH v2 0/6] BTT error clearing rework Vishal Verma 2017-07-17 21:27 ` [PATCH v2 1/6] btt: fix a missed NVDIMM_IO_ATOMIC case in the write path Vishal Verma @ 2017-07-17 21:27 ` Vishal Verma 2017-07-17 21:27 ` [PATCH v2 3/6] btt: ensure that flags were also unchanged during a map_read Vishal Verma ` (3 subsequent siblings) 5 siblings, 0 replies; 11+ messages in thread From: Vishal Verma @ 2017-07-17 21:27 UTC (permalink / raw) To: linux-nvdimm; +Cc: Rafael J. Wysocki Add helpers for converting a raw map entry to just the block number, or either of the 'e' or 'z' flags in preparation for actually using the error flag to mark blocks with media errors. Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/nvdimm/btt.c | 8 ++++---- drivers/nvdimm/btt.h | 4 ++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 52aa96f..3aa6bc0 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -106,7 +106,7 @@ static int btt_map_write(struct arena_info *arena, u32 lba, u32 mapping, * This 'mapping' is supposed to be just the LBA mapping, without * any flags set, so strip the flag bits. */ - mapping &= MAP_LBA_MASK; + mapping = ent_lba(mapping); ze = (z_flag << 1) + e_flag; switch (ze) { @@ -155,10 +155,10 @@ static int btt_map_read(struct arena_info *arena, u32 lba, u32 *mapping, raw_mapping = le32_to_cpu(in); - z_flag = (raw_mapping & MAP_TRIM_MASK) >> MAP_TRIM_SHIFT; - e_flag = (raw_mapping & MAP_ERR_MASK) >> MAP_ERR_SHIFT; + z_flag = ent_z_flag(raw_mapping); + e_flag = ent_e_flag(raw_mapping); ze = (z_flag << 1) + e_flag; - postmap = raw_mapping & MAP_LBA_MASK; + postmap = ent_lba(raw_mapping); /* Reuse the {z,e}_flag variables for *trim and *error */ z_flag = 0; diff --git a/drivers/nvdimm/btt.h b/drivers/nvdimm/btt.h index 888e862..09fabf5 100644 --- a/drivers/nvdimm/btt.h +++ b/drivers/nvdimm/btt.h @@ -38,6 +38,10 @@ #define IB_FLAG_ERROR 0x00000001 #define IB_FLAG_ERROR_MASK 0x00000001 +#define ent_lba(ent) (ent & MAP_LBA_MASK) +#define ent_e_flag(ent) (!!(ent & MAP_ERR_MASK)) +#define ent_z_flag(ent) (!!(ent & MAP_TRIM_MASK)) + enum btt_init_state { INIT_UNCHECKED = 0, INIT_NOTFOUND, -- 2.9.3 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/6] btt: ensure that flags were also unchanged during a map_read 2017-07-17 21:27 [PATCH v2 0/6] BTT error clearing rework Vishal Verma 2017-07-17 21:27 ` [PATCH v2 1/6] btt: fix a missed NVDIMM_IO_ATOMIC case in the write path Vishal Verma 2017-07-17 21:27 ` [PATCH v2 2/6] btt: refactor map entry operations with macros Vishal Verma @ 2017-07-17 21:27 ` Vishal Verma 2017-07-17 21:27 ` [PATCH v2 4/6] btt: cache sector_size in arena_info Vishal Verma ` (2 subsequent siblings) 5 siblings, 0 replies; 11+ messages in thread From: Vishal Verma @ 2017-07-17 21:27 UTC (permalink / raw) To: linux-nvdimm; +Cc: Rafael J. Wysocki In btt_map_read, we read the map twice to make sure that the map entry didn't change after we added it to the read tracking table. In anticipation of expanding the use of the error bit, also make sure that the error and zero flags are constant across the two map reads. Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/nvdimm/btt.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 3aa6bc0..304dac4 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -1031,6 +1031,7 @@ static int btt_read_pg(struct btt *btt, struct bio_integrity_payload *bip, */ while (1) { u32 new_map; + int new_t, new_e; if (t_flag) { zero_fill_data(page, off, cur_len); @@ -1049,15 +1050,18 @@ static int btt_read_pg(struct btt *btt, struct bio_integrity_payload *bip, */ barrier(); - ret = btt_map_read(arena, premap, &new_map, &t_flag, - &e_flag, NVDIMM_IO_ATOMIC); + ret = btt_map_read(arena, premap, &new_map, &new_t, + &new_e, NVDIMM_IO_ATOMIC); if (ret) goto out_rtt; - if (postmap == new_map) + if ((postmap == new_map) && (t_flag == new_t) && + (e_flag == new_e)) break; postmap = new_map; + t_flag = new_t; + e_flag = new_e; } ret = btt_data_read(arena, page, off, postmap, cur_len); -- 2.9.3 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/6] btt: cache sector_size in arena_info 2017-07-17 21:27 [PATCH v2 0/6] BTT error clearing rework Vishal Verma ` (2 preceding siblings ...) 2017-07-17 21:27 ` [PATCH v2 3/6] btt: ensure that flags were also unchanged during a map_read Vishal Verma @ 2017-07-17 21:27 ` Vishal Verma 2017-07-17 21:27 ` [PATCH v2 5/6] libnvdimm, btt: rework error clearing Vishal Verma 2017-07-17 21:27 ` Vishal Verma 5 siblings, 0 replies; 11+ messages in thread From: Vishal Verma @ 2017-07-17 21:27 UTC (permalink / raw) To: linux-nvdimm; +Cc: Rafael J. Wysocki In preparation for the error clearing rework, add sector_size in the arena_info struct. Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/nvdimm/btt.c | 1 + drivers/nvdimm/btt.h | 2 ++ 2 files changed, 3 insertions(+) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 304dac4..6b84eae 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -566,6 +566,7 @@ static struct arena_info *alloc_arena(struct btt *btt, size_t size, if (!arena) return NULL; arena->nd_btt = btt->nd_btt; + arena->sector_size = btt->sector_size; if (!size) return arena; diff --git a/drivers/nvdimm/btt.h b/drivers/nvdimm/btt.h index 09fabf5..2bc0d10b 100644 --- a/drivers/nvdimm/btt.h +++ b/drivers/nvdimm/btt.h @@ -108,6 +108,7 @@ struct aligned_lock { * handle incoming writes. * @version_major: Metadata layout version major. * @version_minor: Metadata layout version minor. + * @sector_size: The Linux sector size - 512 or 4096 * @nextoff: Offset in bytes to the start of the next arena. * @infooff: Offset in bytes to the info block of this arena. * @dataoff: Offset in bytes to the data area of this arena. @@ -135,6 +136,7 @@ struct arena_info { u32 nfree; u16 version_major; u16 version_minor; + u32 sector_size; /* Byte offsets to the different on-media structures */ u64 nextoff; u64 infooff; -- 2.9.3 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 5/6] libnvdimm, btt: rework error clearing 2017-07-17 21:27 [PATCH v2 0/6] BTT error clearing rework Vishal Verma ` (3 preceding siblings ...) 2017-07-17 21:27 ` [PATCH v2 4/6] btt: cache sector_size in arena_info Vishal Verma @ 2017-07-17 21:27 ` Vishal Verma 2017-07-17 22:00 ` Dan Williams 2017-07-17 21:27 ` Vishal Verma 5 siblings, 1 reply; 11+ messages in thread From: Vishal Verma @ 2017-07-17 21:27 UTC (permalink / raw) To: linux-nvdimm; +Cc: Rafael J. Wysocki Clearing errors or badblocks during a BTT write requires sending an ACPI DSM, which means potentially sleeping. Since a BTT IO happens in atomic context (preemption disabled, spinlocks may be held), we cannot perform error clearing in the course of an IO. Due to this error clearing for BTT IOs has hitherto been disabled. In this patch we move error clearing out of the atomic section, and thus re-enable error clearing with BTTs. When we are about to add a block to the free list, we check if it was previously marked as an error, and if it was, we add it to the freelist, but also set a flag that says error clearing will be required. We then drop the lane (ending the atomic context), and send a zero buffer so that the error can be cleared. The error flag in the free list is protected by the nd 'lane', and is set only be a thread while it holds that lane. When the error is cleared, the flag is cleared, but while holding a mutex for that freelist index. When writing, we check for two things - 1/ If the freelist mutex is held or if the error flag is set. If so, this is an error block that is being (or about to be) cleared. 2/ If the block is a known badblock based on nsio->bb The second check is required because the BTT map error flag for a map entry only gets set when an error LBA is read. If we write to a new location that may not have the map error flag set, but still might be in the region's badblock list, we can trigger an EIO on the write, which is undesirable and completely avoidable. Cc: Jeff Moyer <jmoyer@redhat.com> Cc: Toshi Kani <toshi.kani@hpe.com> Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/nvdimm/btt.c | 89 +++++++++++++++++++++++++++++++++++++++++++++----- drivers/nvdimm/btt.h | 5 +++ drivers/nvdimm/claim.c | 8 ----- 3 files changed, 85 insertions(+), 17 deletions(-) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 6b84eae..48382ca9 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -381,7 +381,9 @@ static int btt_flog_write(struct arena_info *arena, u32 lane, u32 sub, arena->freelist[lane].sub = 1 - arena->freelist[lane].sub; if (++(arena->freelist[lane].seq) == 4) arena->freelist[lane].seq = 1; - arena->freelist[lane].block = le32_to_cpu(ent->old_map); + if (ent_e_flag(ent->old_map)) + arena->freelist[lane].has_err = 1; + arena->freelist[lane].block = le32_to_cpu(ent_lba(ent->old_map)); return ret; } @@ -480,6 +482,34 @@ static int btt_log_init(struct arena_info *arena) return ret; } +static u64 to_namespace_offset(struct arena_info *arena, u64 lba) +{ + return arena->dataoff + ((u64)lba * arena->internal_lbasize); +} + +static int arena_clear_freelist_error(struct arena_info *arena, u32 lane) +{ + int ret = 0; + + if (arena->freelist[lane].has_err) { + u32 lba = arena->freelist[lane].block; + u64 nsoff = to_namespace_offset(arena, lba); + void *zerobuf; + + zerobuf = kzalloc(arena->sector_size, GFP_KERNEL); + if (!zerobuf) + return -ENOMEM; + + mutex_lock(&arena->freelist[lane].err_lock); + ret = arena_write_bytes(arena, nsoff, zerobuf, + arena->sector_size, 0); + arena->freelist[lane].has_err = 0; + mutex_unlock(&arena->freelist[lane].err_lock); + kfree(zerobuf); + } + return ret; +} + static int btt_freelist_init(struct arena_info *arena) { int old, new, ret; @@ -505,6 +535,9 @@ static int btt_freelist_init(struct arena_info *arena) arena->freelist[i].seq = nd_inc_seq(le32_to_cpu(log_new.seq)); arena->freelist[i].block = le32_to_cpu(log_new.old_map); + if (ent_e_flag(log_new.old_map)) + arena_clear_freelist_error(arena, i); + /* This implies a newly created or untouched flog entry */ if (log_new.old_map == log_new.new_map) continue; @@ -525,7 +558,7 @@ static int btt_freelist_init(struct arena_info *arena) if (ret) return ret; } - + mutex_init(&arena->freelist[i].err_lock); } return 0; @@ -905,11 +938,6 @@ static void unlock_map(struct arena_info *arena, u32 premap) spin_unlock(&arena->map_locks[idx].lock); } -static u64 to_namespace_offset(struct arena_info *arena, u64 lba) -{ - return arena->dataoff + ((u64)lba * arena->internal_lbasize); -} - static int btt_data_read(struct arena_info *arena, struct page *page, unsigned int off, u32 lba, u32 len) { @@ -1066,8 +1094,14 @@ static int btt_read_pg(struct btt *btt, struct bio_integrity_payload *bip, } ret = btt_data_read(arena, page, off, postmap, cur_len); - if (ret) + if (ret) { + int rc; + + /* Media error - set the e_flag */ + rc = btt_map_write(arena, premap, postmap, 0, 1, + NVDIMM_IO_ATOMIC); goto out_rtt; + } if (bip) { ret = btt_rw_integrity(btt, bip, arena, postmap, READ); @@ -1092,6 +1126,15 @@ static int btt_read_pg(struct btt *btt, struct bio_integrity_payload *bip, return ret; } +static bool btt_is_badblock(struct btt *btt, struct arena_info *arena, + u32 postmap) +{ + u64 nsoff = to_namespace_offset(arena, postmap); + sector_t phys_sector = nsoff >> 9; + + return is_bad_pmem(btt->phys_bb, phys_sector, arena->internal_lbasize); +} + static int btt_write_pg(struct btt *btt, struct bio_integrity_payload *bip, sector_t sector, struct page *page, unsigned int off, unsigned int len) @@ -1104,7 +1147,9 @@ static int btt_write_pg(struct btt *btt, struct bio_integrity_payload *bip, while (len) { u32 cur_len; + int e_flag; + retry: lane = nd_region_acquire_lane(btt->nd_region); ret = lba_to_arena(btt, sector, &premap, &arena); @@ -1117,6 +1162,24 @@ static int btt_write_pg(struct btt *btt, struct bio_integrity_payload *bip, goto out_lane; } + /* The block had a media error, and is being cleared */ + if (mutex_is_locked(&arena->freelist[lane].err_lock) + || arena->freelist[lane].has_err) { + nd_region_release_lane(btt->nd_region, lane); + /* OK to acquire a different lane/free block */ + goto retry; + } + + /* The block had a media error, and needs to be cleared */ + if (btt_is_badblock(btt, arena, arena->freelist[lane].block)) { + arena->freelist[lane].has_err = 1; + nd_region_release_lane(btt->nd_region, lane); + + arena_clear_freelist_error(arena, lane); + /* OK to acquire a different lane/free block */ + goto retry; + } + new_postmap = arena->freelist[lane].block; /* Wait if the new block is being read from */ @@ -1142,7 +1205,7 @@ static int btt_write_pg(struct btt *btt, struct bio_integrity_payload *bip, } lock_map(arena, premap); - ret = btt_map_read(arena, premap, &old_postmap, NULL, NULL, + ret = btt_map_read(arena, premap, &old_postmap, NULL, &e_flag, NVDIMM_IO_ATOMIC); if (ret) goto out_map; @@ -1150,6 +1213,8 @@ static int btt_write_pg(struct btt *btt, struct bio_integrity_payload *bip, ret = -EIO; goto out_map; } + if (e_flag) + set_e_flag(old_postmap); log.lba = cpu_to_le32(premap); log.old_map = cpu_to_le32(old_postmap); @@ -1168,6 +1233,9 @@ static int btt_write_pg(struct btt *btt, struct bio_integrity_payload *bip, unlock_map(arena, premap); nd_region_release_lane(btt->nd_region, lane); + if (e_flag) + arena_clear_freelist_error(arena, lane); + len -= cur_len; off += cur_len; sector += btt->sector_size >> SECTOR_SHIFT; @@ -1358,6 +1426,7 @@ static struct btt *btt_init(struct nd_btt *nd_btt, unsigned long long rawsize, { int ret; struct btt *btt; + struct nd_namespace_io *nsio; struct device *dev = &nd_btt->dev; btt = devm_kzalloc(dev, sizeof(struct btt), GFP_KERNEL); @@ -1371,6 +1440,8 @@ static struct btt *btt_init(struct nd_btt *nd_btt, unsigned long long rawsize, INIT_LIST_HEAD(&btt->arena_list); mutex_init(&btt->init_lock); btt->nd_region = nd_region; + nsio = to_nd_namespace_io(&nd_btt->ndns->dev); + btt->phys_bb = &nsio->bb; ret = discover_arenas(btt); if (ret) { diff --git a/drivers/nvdimm/btt.h b/drivers/nvdimm/btt.h index 2bc0d10b..69c1f90 100644 --- a/drivers/nvdimm/btt.h +++ b/drivers/nvdimm/btt.h @@ -15,6 +15,7 @@ #ifndef _LINUX_BTT_H #define _LINUX_BTT_H +#include <linux/badblocks.h> #include <linux/types.h> #define BTT_SIG_LEN 16 @@ -41,6 +42,7 @@ #define ent_lba(ent) (ent & MAP_LBA_MASK) #define ent_e_flag(ent) (!!(ent & MAP_ERR_MASK)) #define ent_z_flag(ent) (!!(ent & MAP_TRIM_MASK)) +#define set_e_flag(ent) (ent |= MAP_ERR_MASK) enum btt_init_state { INIT_UNCHECKED = 0, @@ -82,6 +84,8 @@ struct free_entry { u32 block; u8 sub; u8 seq; + struct mutex err_lock; + u8 has_err; }; struct aligned_lock { @@ -187,6 +191,7 @@ struct btt { struct mutex init_lock; int init_state; int num_arenas; + struct badblocks *phys_bb; }; bool nd_btt_arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super); diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c index 8f29937..727f11b 100644 --- a/drivers/nvdimm/claim.c +++ b/drivers/nvdimm/claim.c @@ -280,14 +280,6 @@ static int nsio_rw_bytes(struct nd_namespace_common *ndns, } if (unlikely(is_bad_pmem(&nsio->bb, sector, sz_align))) { - /* - * FIXME: nsio_rw_bytes() may be called from atomic - * context in the btt case and the ACPI DSM path for - * clearing the error takes sleeping locks and allocates - * memory. An explicit error clearing path, and support - * for tracking badblocks in BTT metadata is needed to - * work around this collision. - */ if (IS_ALIGNED(offset, 512) && IS_ALIGNED(size, 512) && !(flags & NVDIMM_IO_ATOMIC)) { long cleared; -- 2.9.3 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 5/6] libnvdimm, btt: rework error clearing 2017-07-17 21:27 ` [PATCH v2 5/6] libnvdimm, btt: rework error clearing Vishal Verma @ 2017-07-17 22:00 ` Dan Williams 2017-07-17 22:20 ` Verma, Vishal L 0 siblings, 1 reply; 11+ messages in thread From: Dan Williams @ 2017-07-17 22:00 UTC (permalink / raw) To: Vishal Verma; +Cc: Rafael J. Wysocki, linux-nvdimm@lists.01.org On Mon, Jul 17, 2017 at 2:27 PM, Vishal Verma <vishal.l.verma@intel.com> wrote: > Clearing errors or badblocks during a BTT write requires sending an ACPI > DSM, which means potentially sleeping. Since a BTT IO happens in atomic > context (preemption disabled, spinlocks may be held), we cannot perform > error clearing in the course of an IO. Due to this error clearing for > BTT IOs has hitherto been disabled. > > In this patch we move error clearing out of the atomic section, and thus > re-enable error clearing with BTTs. When we are about to add a block to > the free list, we check if it was previously marked as an error, and if > it was, we add it to the freelist, but also set a flag that says error > clearing will be required. We then drop the lane (ending the atomic > context), and send a zero buffer so that the error can be cleared. The > error flag in the free list is protected by the nd 'lane', and is set > only be a thread while it holds that lane. When the error is cleared, > the flag is cleared, but while holding a mutex for that freelist index. > > When writing, we check for two things - > 1/ If the freelist mutex is held or if the error flag is set. If so, > this is an error block that is being (or about to be) cleared. > 2/ If the block is a known badblock based on nsio->bb > > The second check is required because the BTT map error flag for a map > entry only gets set when an error LBA is read. If we write to a new > location that may not have the map error flag set, but still might be in > the region's badblock list, we can trigger an EIO on the write, which is > undesirable and completely avoidable. > > Cc: Jeff Moyer <jmoyer@redhat.com> > Cc: Toshi Kani <toshi.kani@hpe.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > drivers/nvdimm/btt.c | 89 +++++++++++++++++++++++++++++++++++++++++++++----- > drivers/nvdimm/btt.h | 5 +++ > drivers/nvdimm/claim.c | 8 ----- > 3 files changed, 85 insertions(+), 17 deletions(-) > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > index 6b84eae..48382ca9 100644 > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -381,7 +381,9 @@ static int btt_flog_write(struct arena_info *arena, u32 lane, u32 sub, > arena->freelist[lane].sub = 1 - arena->freelist[lane].sub; > if (++(arena->freelist[lane].seq) == 4) > arena->freelist[lane].seq = 1; > - arena->freelist[lane].block = le32_to_cpu(ent->old_map); > + if (ent_e_flag(ent->old_map)) > + arena->freelist[lane].has_err = 1; > + arena->freelist[lane].block = le32_to_cpu(ent_lba(ent->old_map)); > > return ret; > } > @@ -480,6 +482,34 @@ static int btt_log_init(struct arena_info *arena) > return ret; > } > > +static u64 to_namespace_offset(struct arena_info *arena, u64 lba) > +{ > + return arena->dataoff + ((u64)lba * arena->internal_lbasize); > +} > + > +static int arena_clear_freelist_error(struct arena_info *arena, u32 lane) > +{ > + int ret = 0; > + > + if (arena->freelist[lane].has_err) { > + u32 lba = arena->freelist[lane].block; > + u64 nsoff = to_namespace_offset(arena, lba); > + void *zerobuf; > + > + zerobuf = kzalloc(arena->sector_size, GFP_KERNEL); > + if (!zerobuf) > + return -ENOMEM; Hmm, can this just use the system zero page (empty_zero_page)? Also, even if it can't, I'd prefer the NOIO fix for this case be folded into this patch. The NOIO fixup for the acpi dsm path should be separate. Another consideration if we can't use the system zero page, do we need to allocate this on demand? Seems like something we could just pre-allocate at init time. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 5/6] libnvdimm, btt: rework error clearing 2017-07-17 22:00 ` Dan Williams @ 2017-07-17 22:20 ` Verma, Vishal L 2017-07-17 22:32 ` Dan Williams 0 siblings, 1 reply; 11+ messages in thread From: Verma, Vishal L @ 2017-07-17 22:20 UTC (permalink / raw) To: Williams, Dan J; +Cc: Wysocki, Rafael J, linux-nvdimm@lists.01.org On Mon, 2017-07-17 at 15:00 -0700, Dan Williams wrote: > On Mon, Jul 17, 2017 at 2:27 PM, Vishal Verma <vishal.l.verma@intel.co > m> wrote: > > Clearing errors or badblocks during a BTT write requires sending an > > ACPI > > DSM, which means potentially sleeping. Since a BTT IO happens in > > atomic > > context (preemption disabled, spinlocks may be held), we cannot > > perform > > error clearing in the course of an IO. Due to this error clearing > > for > > BTT IOs has hitherto been disabled. > > > > In this patch we move error clearing out of the atomic section, and > > thus > > re-enable error clearing with BTTs. When we are about to add a block > > to > > the free list, we check if it was previously marked as an error, and > > if > > it was, we add it to the freelist, but also set a flag that says > > error > > clearing will be required. We then drop the lane (ending the atomic > > context), and send a zero buffer so that the error can be cleared. > > The > > error flag in the free list is protected by the nd 'lane', and is > > set > > only be a thread while it holds that lane. When the error is > > cleared, > > the flag is cleared, but while holding a mutex for that freelist > > index. > > > > When writing, we check for two things - > > 1/ If the freelist mutex is held or if the error flag is set. If so, > > this is an error block that is being (or about to be) cleared. > > 2/ If the block is a known badblock based on nsio->bb > > > > The second check is required because the BTT map error flag for a > > map > > entry only gets set when an error LBA is read. If we write to a new > > location that may not have the map error flag set, but still might > > be in > > the region's badblock list, we can trigger an EIO on the write, > > which is > > undesirable and completely avoidable. > > > > Cc: Jeff Moyer <jmoyer@redhat.com> > > Cc: Toshi Kani <toshi.kani@hpe.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > --- > > drivers/nvdimm/btt.c | 89 > > +++++++++++++++++++++++++++++++++++++++++++++----- > > drivers/nvdimm/btt.h | 5 +++ > > drivers/nvdimm/claim.c | 8 ----- > > 3 files changed, 85 insertions(+), 17 deletions(-) > > <> > > +static int arena_clear_freelist_error(struct arena_info *arena, u32 > > lane) > > +{ > > + int ret = 0; > > + > > + if (arena->freelist[lane].has_err) { > > + u32 lba = arena->freelist[lane].block; > > + u64 nsoff = to_namespace_offset(arena, lba); > > + void *zerobuf; > > + > > + zerobuf = kzalloc(arena->sector_size, GFP_KERNEL); > > + if (!zerobuf) > > + return -ENOMEM; > > Hmm, can this just use the system zero page (empty_zero_page)? Also, > even if it can't, I'd prefer the NOIO fix for this case be folded into > this patch. The NOIO fixup for the acpi dsm path should be separate. > > Another consideration if we can't use the system zero page, do we need > to allocate this on demand? Seems like something we could just > pre-allocate at init time. I had started out using the empty_zero_page, but to use that you have to kmap_atomic the page, which ends up creating an atomic section again :) Sounds good on preallocating a zero page at init. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 5/6] libnvdimm, btt: rework error clearing 2017-07-17 22:20 ` Verma, Vishal L @ 2017-07-17 22:32 ` Dan Williams 0 siblings, 0 replies; 11+ messages in thread From: Dan Williams @ 2017-07-17 22:32 UTC (permalink / raw) To: Verma, Vishal L; +Cc: Wysocki, Rafael J, linux-nvdimm@lists.01.org On Mon, Jul 17, 2017 at 3:20 PM, Verma, Vishal L <vishal.l.verma@intel.com> wrote: > On Mon, 2017-07-17 at 15:00 -0700, Dan Williams wrote: >> On Mon, Jul 17, 2017 at 2:27 PM, Vishal Verma <vishal.l.verma@intel.co >> m> wrote: >> > Clearing errors or badblocks during a BTT write requires sending an >> > ACPI >> > DSM, which means potentially sleeping. Since a BTT IO happens in >> > atomic >> > context (preemption disabled, spinlocks may be held), we cannot >> > perform >> > error clearing in the course of an IO. Due to this error clearing >> > for >> > BTT IOs has hitherto been disabled. >> > >> > In this patch we move error clearing out of the atomic section, and >> > thus >> > re-enable error clearing with BTTs. When we are about to add a block >> > to >> > the free list, we check if it was previously marked as an error, and >> > if >> > it was, we add it to the freelist, but also set a flag that says >> > error >> > clearing will be required. We then drop the lane (ending the atomic >> > context), and send a zero buffer so that the error can be cleared. >> > The >> > error flag in the free list is protected by the nd 'lane', and is >> > set >> > only be a thread while it holds that lane. When the error is >> > cleared, >> > the flag is cleared, but while holding a mutex for that freelist >> > index. >> > >> > When writing, we check for two things - >> > 1/ If the freelist mutex is held or if the error flag is set. If so, >> > this is an error block that is being (or about to be) cleared. >> > 2/ If the block is a known badblock based on nsio->bb >> > >> > The second check is required because the BTT map error flag for a >> > map >> > entry only gets set when an error LBA is read. If we write to a new >> > location that may not have the map error flag set, but still might >> > be in >> > the region's badblock list, we can trigger an EIO on the write, >> > which is >> > undesirable and completely avoidable. >> > >> > Cc: Jeff Moyer <jmoyer@redhat.com> >> > Cc: Toshi Kani <toshi.kani@hpe.com> >> > Cc: Dan Williams <dan.j.williams@intel.com> >> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> >> > --- >> > drivers/nvdimm/btt.c | 89 >> > +++++++++++++++++++++++++++++++++++++++++++++----- >> > drivers/nvdimm/btt.h | 5 +++ >> > drivers/nvdimm/claim.c | 8 ----- >> > 3 files changed, 85 insertions(+), 17 deletions(-) >> > > > <> > >> > +static int arena_clear_freelist_error(struct arena_info *arena, u32 >> > lane) >> > +{ >> > + int ret = 0; >> > + >> > + if (arena->freelist[lane].has_err) { >> > + u32 lba = arena->freelist[lane].block; >> > + u64 nsoff = to_namespace_offset(arena, lba); >> > + void *zerobuf; >> > + >> > + zerobuf = kzalloc(arena->sector_size, GFP_KERNEL); >> > + if (!zerobuf) >> > + return -ENOMEM; >> >> Hmm, can this just use the system zero page (empty_zero_page)? Also, >> even if it can't, I'd prefer the NOIO fix for this case be folded into >> this patch. The NOIO fixup for the acpi dsm path should be separate. >> >> Another consideration if we can't use the system zero page, do we need >> to allocate this on demand? Seems like something we could just >> pre-allocate at init time. > > I had started out using the empty_zero_page, but to use that you have to > kmap_atomic the page, which ends up creating an atomic section again :) I think it is safe to assume that the zero page is never in highmem. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 6/6] pmem, btt: fix potential deadlock while clearing errors 2017-07-17 21:27 [PATCH v2 0/6] BTT error clearing rework Vishal Verma @ 2017-07-17 21:27 ` Vishal Verma 2017-07-17 21:27 ` [PATCH v2 2/6] btt: refactor map entry operations with macros Vishal Verma ` (4 subsequent siblings) 5 siblings, 0 replies; 11+ messages in thread From: Vishal Verma @ 2017-07-17 21:27 UTC (permalink / raw) To: linux-nvdimm Cc: Dan Williams, Jeff Moyer, Rafael J. Wysocki, Toshi Kani, Vishal Verma, linux-acpi, Robert Moore With the ACPI NFIT 'DSM' methods, acpi can be called from IO paths. Specifically, the DSM to clear media errors is called during writes, so that we can provide a writes-fix-errors model. However it is easy to imagine a scenario like: -> write through the nvdimm driver -> acpi allocation -> writeback, causes more IO through the nvdimm driver -> deadlock Fix this by using memalloc_noio_{save,restore}, which sets the GFP_NOIO flag for the current scope when issuing commands/IOs that are expected to clear errors. Cc: <linux-acpi@vger.kernel.org> Cc: <linux-nvdimm@lists.01.org> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Robert Moore <robert.moore@intel.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/nvdimm/btt.c | 8 +++++++- drivers/nvdimm/pmem.c | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 48382ca9..25b576d 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -11,6 +11,7 @@ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for * more details. */ +#include <linux/sched/mm.h> #include <linux/highmem.h> #include <linux/debugfs.h> #include <linux/blkdev.h> @@ -494,17 +495,22 @@ static int arena_clear_freelist_error(struct arena_info *arena, u32 lane) if (arena->freelist[lane].has_err) { u32 lba = arena->freelist[lane].block; u64 nsoff = to_namespace_offset(arena, lba); + unsigned int noio_flag; void *zerobuf; + noio_flag = memalloc_noio_save(); zerobuf = kzalloc(arena->sector_size, GFP_KERNEL); - if (!zerobuf) + if (!zerobuf) { + memalloc_noio_restore(noio_flag); return -ENOMEM; + } mutex_lock(&arena->freelist[lane].err_lock); ret = arena_write_bytes(arena, nsoff, zerobuf, arena->sector_size, 0); arena->freelist[lane].has_err = 0; mutex_unlock(&arena->freelist[lane].err_lock); + memalloc_noio_restore(noio_flag); kfree(zerobuf); } return ret; diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 5c45e17..468e032 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -24,6 +24,7 @@ #include <linux/moduleparam.h> #include <linux/badblocks.h> #include <linux/memremap.h> +#include <linux/sched/mm.h> #include <linux/vmalloc.h> #include <linux/blk-mq.h> #include <linux/pfn_t.h> @@ -53,12 +54,14 @@ static int pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset, unsigned int len) { struct device *dev = to_dev(pmem); + unsigned int noio_flag; sector_t sector; long cleared; int rc = 0; sector = (offset - pmem->data_offset) / 512; + noio_flag = memalloc_noio_save(); cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset, len); if (cleared < len) rc = -EIO; @@ -69,6 +72,7 @@ static int pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset, cleared > 1 ? "s" : ""); badblocks_clear(&pmem->bb, sector, cleared); } + memalloc_noio_restore(noio_flag); invalidate_pmem(pmem->virt_addr + offset, len); -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 6/6] pmem, btt: fix potential deadlock while clearing errors @ 2017-07-17 21:27 ` Vishal Verma 0 siblings, 0 replies; 11+ messages in thread From: Vishal Verma @ 2017-07-17 21:27 UTC (permalink / raw) To: linux-nvdimm; +Cc: Rafael J. Wysocki, Robert Moore, linux-acpi With the ACPI NFIT 'DSM' methods, acpi can be called from IO paths. Specifically, the DSM to clear media errors is called during writes, so that we can provide a writes-fix-errors model. However it is easy to imagine a scenario like: -> write through the nvdimm driver -> acpi allocation -> writeback, causes more IO through the nvdimm driver -> deadlock Fix this by using memalloc_noio_{save,restore}, which sets the GFP_NOIO flag for the current scope when issuing commands/IOs that are expected to clear errors. Cc: <linux-acpi@vger.kernel.org> Cc: <linux-nvdimm@lists.01.org> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Robert Moore <robert.moore@intel.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/nvdimm/btt.c | 8 +++++++- drivers/nvdimm/pmem.c | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 48382ca9..25b576d 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -11,6 +11,7 @@ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for * more details. */ +#include <linux/sched/mm.h> #include <linux/highmem.h> #include <linux/debugfs.h> #include <linux/blkdev.h> @@ -494,17 +495,22 @@ static int arena_clear_freelist_error(struct arena_info *arena, u32 lane) if (arena->freelist[lane].has_err) { u32 lba = arena->freelist[lane].block; u64 nsoff = to_namespace_offset(arena, lba); + unsigned int noio_flag; void *zerobuf; + noio_flag = memalloc_noio_save(); zerobuf = kzalloc(arena->sector_size, GFP_KERNEL); - if (!zerobuf) + if (!zerobuf) { + memalloc_noio_restore(noio_flag); return -ENOMEM; + } mutex_lock(&arena->freelist[lane].err_lock); ret = arena_write_bytes(arena, nsoff, zerobuf, arena->sector_size, 0); arena->freelist[lane].has_err = 0; mutex_unlock(&arena->freelist[lane].err_lock); + memalloc_noio_restore(noio_flag); kfree(zerobuf); } return ret; diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 5c45e17..468e032 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -24,6 +24,7 @@ #include <linux/moduleparam.h> #include <linux/badblocks.h> #include <linux/memremap.h> +#include <linux/sched/mm.h> #include <linux/vmalloc.h> #include <linux/blk-mq.h> #include <linux/pfn_t.h> @@ -53,12 +54,14 @@ static int pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset, unsigned int len) { struct device *dev = to_dev(pmem); + unsigned int noio_flag; sector_t sector; long cleared; int rc = 0; sector = (offset - pmem->data_offset) / 512; + noio_flag = memalloc_noio_save(); cleared = nvdimm_clear_poison(dev, pmem->phys_addr + offset, len); if (cleared < len) rc = -EIO; @@ -69,6 +72,7 @@ static int pmem_clear_poison(struct pmem_device *pmem, phys_addr_t offset, cleared > 1 ? "s" : ""); badblocks_clear(&pmem->bb, sector, cleared); } + memalloc_noio_restore(noio_flag); invalidate_pmem(pmem->virt_addr + offset, len); -- 2.9.3 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-07-17 22:30 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-17 21:27 [PATCH v2 0/6] BTT error clearing rework Vishal Verma 2017-07-17 21:27 ` [PATCH v2 1/6] btt: fix a missed NVDIMM_IO_ATOMIC case in the write path Vishal Verma 2017-07-17 21:27 ` [PATCH v2 2/6] btt: refactor map entry operations with macros Vishal Verma 2017-07-17 21:27 ` [PATCH v2 3/6] btt: ensure that flags were also unchanged during a map_read Vishal Verma 2017-07-17 21:27 ` [PATCH v2 4/6] btt: cache sector_size in arena_info Vishal Verma 2017-07-17 21:27 ` [PATCH v2 5/6] libnvdimm, btt: rework error clearing Vishal Verma 2017-07-17 22:00 ` Dan Williams 2017-07-17 22:20 ` Verma, Vishal L 2017-07-17 22:32 ` Dan Williams 2017-07-17 21:27 ` [PATCH v2 6/6] pmem, btt: fix potential deadlock while clearing errors Vishal Verma 2017-07-17 21:27 ` Vishal Verma
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.