Message ID | 20190226231331.8730-1-vishal.l.verma@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v4] nvdimm, btt: fix LBA masking during 'free list' population | expand |
> From: Vishal Verma <vishal.l.verma@intel.com> > Sent: Tuesday, February 26, 2019 3:14 PM > ... > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > @@ -555,16 +555,21 @@ static int btt_freelist_init(struct arena_info *arena) > if (new < 0) > return new; > > + /* old and new map entries with any flags stripped out */ > + log_oldmap = ent_lba(le32_to_cpu(log_new.old_map)); > + log_newmap = ent_lba(le32_to_cpu(log_new.new_map)); > + > /* sub points to the next one to be overwritten */ > arena->freelist[i].sub = 1 - new; > arena->freelist[i].seq = nd_inc_seq(le32_to_cpu(log_new.seq)); > - arena->freelist[i].block = le32_to_cpu(log_new.old_map); > + arena->freelist[i].block = log_oldmap; > > /* > * FIXME: if error clearing fails during init, we want to make > * the BTT read-only > */ > if (ent_e_flag(log_new.old_map)) { > + set_e_flag(arena->freelist[i].block); Hi Vishal, The logic doesn't look quite right to me: as I understand, if both the zero flag and the error flag are set, it means a normal map entry rather than an error. Windows can indeed set both flags: [ 3.756239] freelist_init: i=0, log_old: lba=bcc01, old_map=c00bcc30, new_map=c00bcc02, seq=2 [ 3.765583] freelist_init: i=0, log_new: lba=bcc00, old_map=c0001b7b, new_map=c00bcc30, seq=3 (For the full log, see https://github.com/dcui/linux/commit/b472968e01d72be3bd42e4568befc4602f9ac396 ) Due to this new line, the 'block' can exceed the normal internal_nlba, and cause I/O failure: [ 103.589766] btt_write_pg failed: lane=0x0, new_postmap=0x40001b7b, internal_nlba=0x1ff8018 [ 103.602387] nd_pmem btt1.1: io error in WRITE sector 33056, len 4096, [ 103.611662] Buffer I/O error on dev pmem1s2, logical block 36, lost async page write I suppose the new line should not be added, and the line if (ent_e_flag(log_new.old_map)) { should be changed to if (ent_e_flag(log_new.old_map) && ! ent_z_flag(log_new.old_map)) { ? > ret = arena_clear_freelist_error(arena, i); > if (ret) > dev_err_ratelimited(to_dev(arena), Thanks, -- Dexuan
On Wed, 2019-02-27 at 05:55 +0000, Dexuan Cui wrote: > > From: Vishal Verma <vishal.l.verma@intel.com> > > Sent: Tuesday, February 26, 2019 3:14 PM > > ... > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > > @@ -555,16 +555,21 @@ static int btt_freelist_init(struct arena_info *arena) > > if (new < 0) > > return new; > > > > + /* old and new map entries with any flags stripped out */ > > + log_oldmap = ent_lba(le32_to_cpu(log_new.old_map)); > > + log_newmap = ent_lba(le32_to_cpu(log_new.new_map)); > > + > > /* sub points to the next one to be overwritten */ > > arena->freelist[i].sub = 1 - new; > > arena->freelist[i].seq = nd_inc_seq(le32_to_cpu(log_new.seq)); > > - arena->freelist[i].block = le32_to_cpu(log_new.old_map); > > + arena->freelist[i].block = log_oldmap; > > > > /* > > * FIXME: if error clearing fails during init, we want to make > > * the BTT read-only > > */ > > if (ent_e_flag(log_new.old_map)) { > > + set_e_flag(arena->freelist[i].block); > Hi Vishal, > The logic doesn't look quite right to me: as I understand, if both the zero flag and > the error flag are set, it means a normal map entry rather than an error. > > Windows can indeed set both flags: > [ 3.756239] freelist_init: i=0, log_old: lba=bcc01, old_map=c00bcc30, new_map=c00bcc02, seq=2 > [ 3.765583] freelist_init: i=0, log_new: lba=bcc00, old_map=c0001b7b, new_map=c00bcc30, seq=3 > (For the full log, see https://github.com/dcui/linux/commit/b472968e01d72be3bd42e4568befc4602f9ac396 ) > > Due to this new line, the 'block' can exceed the normal internal_nlba, and cause I/O failure: > > [ 103.589766] btt_write_pg failed: lane=0x0, new_postmap=0x40001b7b, internal_nlba=0x1ff8018 > [ 103.602387] nd_pmem btt1.1: io error in WRITE sector 33056, len 4096, > [ 103.611662] Buffer I/O error on dev pmem1s2, logical block 36, lost async page write > > I suppose the new line should not be added, and the line > if (ent_e_flag(log_new.old_map)) { > should be changed to > if (ent_e_flag(log_new.old_map) && ! ent_z_flag(log_new.old_map)) { > ? > Ah yes good catch, I broke my own rule, in that freelist[i].block should not have flag bits since it is used as is. The freelist has a separate has_err field to allow for error clearing, and we should be setting that. Does the following incremental patch fix it? Let me know and I can send a new version including it. diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index 294c48e45e74..1e7c1a66cef8 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -569,7 +569,7 @@ static int btt_freelist_init(struct arena_info *arena) * the BTT read-only */ if (ent_e_flag(log_new.old_map)) { - set_e_flag(arena->freelist[i].block); + arena->freelist[i].has_err = 1; ret = arena_clear_freelist_error(arena, i); if (ret) dev_err_ratelimited(to_dev(arena),
> From: Verma, Vishal L <vishal.l.verma@intel.com> > Sent: Wednesday, February 27, 2019 10:13 AM > ... > > I suppose the new line should not be added, and the line > > if (ent_e_flag(log_new.old_map)) { > > should be changed to > > if (ent_e_flag(log_new.old_map) && ! > ent_z_flag(log_new.old_map)) { > > ? > > > Ah yes good catch, I broke my own rule, in that freelist[i].block should > not have flag bits since it is used as is. The freelist has a separate > has_err field to allow for error clearing, and we should be setting > that. > > Does the following incremental patch fix it? Let me know and I can send > a new version including it. Yes, the patch works for me. Thank you! But, should we really set the has_err field? Here the enry has both the zere/error flags set. As I understand, this is not an error, because the UEFI spec says "... both Error and Zero are set to indicate a map entry in its normal, non-error state". > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > index 294c48e45e74..1e7c1a66cef8 100644 > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -569,7 +569,7 @@ static int btt_freelist_init(struct arena_info > *arena) > * the BTT read-only > */ > if (ent_e_flag(log_new.old_map)) { > - set_e_flag(arena->freelist[i].block); > + arena->freelist[i].has_err = 1; > ret = arena_clear_freelist_error(arena, i); > if (ret) > dev_err_ratelimited(to_dev(arena), Thanks, -- Dexuan
On Wed, 2019-02-27 at 21:06 +0000, Dexuan Cui wrote: > > From: Verma, Vishal L <vishal.l.verma@intel.com> > > Sent: Wednesday, February 27, 2019 10:13 AM > > ... > > > I suppose the new line should not be added, and the line > > > if (ent_e_flag(log_new.old_map)) { > > > should be changed to > > > if (ent_e_flag(log_new.old_map) && ! > > ent_z_flag(log_new.old_map)) { > > > ? > > > > > Ah yes good catch, I broke my own rule, in that freelist[i].block should > > not have flag bits since it is used as is. The freelist has a separate > > has_err field to allow for error clearing, and we should be setting > > that. > > > > Does the following incremental patch fix it? Let me know and I can send > > a new version including it. > > Yes, the patch works for me. Thank you! > > But, should we really set the has_err field? Here the enry has both > the zere/error flags set. As I understand, this is not an error, because > the UEFI spec says "... both Error and Zero are set to indicate a map entry > in its normal, non-error state". > Ah yes I see what you mean - we'd be pointlessly trying to clear those blocks. I'll fix this in v5.
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index cd4fa87ea48c..294c48e45e74 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -542,8 +542,8 @@ static int arena_clear_freelist_error(struct arena_info *arena, u32 lane) static int btt_freelist_init(struct arena_info *arena) { int new, ret; - u32 i, map_entry; struct log_entry log_new; + u32 i, map_entry, log_oldmap, log_newmap; arena->freelist = kcalloc(arena->nfree, sizeof(struct free_entry), GFP_KERNEL); @@ -555,16 +555,21 @@ static int btt_freelist_init(struct arena_info *arena) if (new < 0) return new; + /* old and new map entries with any flags stripped out */ + log_oldmap = ent_lba(le32_to_cpu(log_new.old_map)); + log_newmap = ent_lba(le32_to_cpu(log_new.new_map)); + /* sub points to the next one to be overwritten */ arena->freelist[i].sub = 1 - new; arena->freelist[i].seq = nd_inc_seq(le32_to_cpu(log_new.seq)); - arena->freelist[i].block = le32_to_cpu(log_new.old_map); + arena->freelist[i].block = log_oldmap; /* * FIXME: if error clearing fails during init, we want to make * the BTT read-only */ if (ent_e_flag(log_new.old_map)) { + set_e_flag(arena->freelist[i].block); ret = arena_clear_freelist_error(arena, i); if (ret) dev_err_ratelimited(to_dev(arena), @@ -572,7 +577,7 @@ static int btt_freelist_init(struct arena_info *arena) } /* This implies a newly created or untouched flog entry */ - if (log_new.old_map == log_new.new_map) + if (log_oldmap == log_newmap) continue; /* Check if map recovery is needed */ @@ -580,8 +585,15 @@ static int btt_freelist_init(struct arena_info *arena) NULL, NULL, 0); if (ret) return ret; - if ((le32_to_cpu(log_new.new_map) != map_entry) && - (le32_to_cpu(log_new.old_map) == map_entry)) { + + /* + * The map_entry from btt_read_map is stripped of any flag bits, + * so use the stripped out versions from the log as well for + * testing whether recovery is needed. For restoration, use the + * 'raw' version of the log entries as that captured what we + * were going to write originally. + */ + if ((log_newmap != map_entry) && (log_oldmap == map_entry)) { /* * Last transaction wrote the flog, but wasn't able * to complete the map write. So fix up the map. diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c index 795ad4ff35ca..b72a303176c7 100644 --- a/drivers/nvdimm/btt_devs.c +++ b/drivers/nvdimm/btt_devs.c @@ -159,11 +159,19 @@ static ssize_t size_show(struct device *dev, } static DEVICE_ATTR_RO(size); +static ssize_t log_zero_flags_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "Y\n"); +} +static DEVICE_ATTR_RO(log_zero_flags); + static struct attribute *nd_btt_attributes[] = { &dev_attr_sector_size.attr, &dev_attr_namespace.attr, &dev_attr_uuid.attr, &dev_attr_size.attr, + &dev_attr_log_zero_flags.attr, NULL, };