On Thu, May 4, 2017 at 2:49 PM, Dave Jiang <[email protected]> wrote:
> Adding ndctl support that will allow clearing of bad blocks for a device.
> Initial implementation will only support device dax devices. The ndctl
> takes a device path and parameters of the starting bad block, and the number
> of bad blocks to clear.
>
> Signed-off-by: Dave Jiang <[email protected]>
> ---
> Documentation/ndctl-clear-error.txt | 38 ++++++
> builtin.h | 1
> ndctl/Makefile.am | 1
> ndctl/clear-error.c | 239
> +++++++++++++++++++++++++++++++++++
> ndctl/lib/libndctl.c | 72 +++++++++++
> ndctl/lib/libndctl.sym | 2
> ndctl/libndctl.h.in | 10 +
> ndctl/ndctl.c | 1
> 8 files changed, 364 insertions(+)
> create mode 100644 Documentation/ndctl-clear-error.txt
> create mode 100644 ndctl/clear-error.c
>
> diff --git a/Documentation/ndctl-clear-error.txt
> b/Documentation/ndctl-clear-error.txt
> new file mode 100644
> index 0000000..b14521a
> --- /dev/null
> +++ b/Documentation/ndctl-clear-error.txt
> @@ -0,0 +1,38 @@
> +ndctl-clear-error(1)
> +====================
> +
> +NAME
> +----
> +ndctl-clear-error - clear badblocks for a device
I think "clear-error" is too ambiguous of a name, lets call the
commands repair-media-errors and list-media-errors.
I'd also like to see the list-media-errors patch first in the series
since repairing is just an incremental step on top of listing. I don't
think the word "badblocks" should appear anywhere in this document.
That's a kernel implementation detail.
The other benefit of "repair" vs "clear" is communicating that the
data may be indeterminate after repair. Hopefully in the future we'll
get a standard mechanism to determine if the platform supports atomic
error clearing with a determinate value.
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'ndctl clear-error' [<options>]
> +
> +EXAMPLES
> +--------
> +
> +Clear poison (bad blocks) for the provided device
> +[verse]
> +ndctl clear-error -f /dev/dax0.0 -s 0 -l 8
> +
> +Clear poison (bad blocks) at block offset 0 for 8 blocks on device
> /dev/dax0.0
The "poison" term is x86 specific. Lets just use "media error" everywhere.
> +
> +OPTIONS
> +-------
> +-f::
> +--file::
> + The device/file to be cleared of poison (bad blocks).
Let's make this --d/--device and drop the "file" option. For files on
filesystems there's other ways to clear errors and I wouldn't
necessarily want them to use this tool. Should also make it clear that
the device argument must refer to a device in an nvdimm bus hierarchy.
> +
> +-s::
> +--start::
> + The offset where the poison (bad block) starts for this device.
> + Typically this is acquired from the sysfs badblocks file.
I assume this is in terms of 512-byte blocks, but we should clarify
the units. The second sentence can be reworded to recommend using the
list-media-errors command. Lets call the option -o / --offset and also
allow a "-o all" to repair all errors reported by list-media-errors.
> +
> +-l::
> +--len::
> + The number of badblocks to clear in size of 512 bytes increments. The
> + length must fit within the badblocks range. If the length exceeds the
> + badblock range or is 0, the command will fail. Not providing a len
> + will result in a single block being cleared.
s/badblocks/media error/
s/--len/--length/
> diff --git a/builtin.h b/builtin.h
> index a8bc848..f522d00 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -30,4 +30,5 @@ int cmd_test(int argc, const char **argv, void *ctx);
> #ifdef ENABLE_DESTRUCTIVE
> int cmd_bat(int argc, const char **argv, void *ctx);
> #endif
> +int cmd_clear_error(int argc, const char **argv, void *ctx);
> #endif /* _NDCTL_BUILTIN_H_ */
> diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> index d346c04..8123169 100644
> --- a/ndctl/Makefile.am
> +++ b/ndctl/Makefile.am
> @@ -11,6 +11,7 @@ ndctl_SOURCES = ndctl.c \
> ../util/log.c \
> list.c \
> test.c \
> + clear-error.c \
> ../util/json.c
>
> if ENABLE_SMART
> diff --git a/ndctl/clear-error.c b/ndctl/clear-error.c
> new file mode 100644
> index 0000000..29cd471
> --- /dev/null
> +++ b/ndctl/clear-error.c
> @@ -0,0 +1,239 @@
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +#include <libgen.h>
> +#include <string.h>
> +#include <limits.h>
> +#include <ccan/short_types/short_types.h>
> +#include <ccan/array_size/array_size.h>
> +#include <util/parse-options.h>
> +#include <util/log.h>
> +#include <ndctl/libndctl.h>
> +#include <ndctl.h>
> +
> +struct clear_err {
> + const char *dev_name;
> + u64 bb_start;
> + unsigned int bb_len;
> + struct ndctl_cmd *ars_cap;
> + struct ndctl_cmd *clear_err;
> + struct ndctl_bus *bus;
> + struct ndctl_region *region;
> + struct ndctl_dax *dax;
> + struct ndctl_ctx *ctx;
> +} clear_err = {
> + .bb_len = 1,
> +};
> +
> +static int send_clear_error(struct ndctl_bus *bus, u64 start, u64 size)
> +{
> + u64 cleared;
> + int rc;
> +
> + clear_err.clear_err = ndctl_bus_cmd_new_clear_error(
> + start, size, clear_err.ars_cap);
> + if (!clear_err.clear_err) {
> + error("%s: bus: %s failed to create cmd\n",
> + __func__, ndctl_bus_get_provider(bus));
> + return -ENXIO;
> + }
> +
> + rc = ndctl_cmd_submit(clear_err.clear_err);
> + if (rc) {
> + error("%s: bus: %s failed to submit cmd: %d\n",
> + __func__, ndctl_bus_get_provider(bus), rc);
> + ndctl_cmd_unref(clear_err.clear_err);
> + return rc;
> + }
> +
> + cleared = ndctl_cmd_clear_error_get_cleared(clear_err.clear_err);
> + if (cleared != size) {
> + error("%s: bus: %s expected to clear: %ld actual: %ld\n",
> + __func__, ndctl_bus_get_provider(bus),
> + size, cleared);
> + return -ENXIO;
> + }
> +
> + return 0;
> +}
> +
> +static int get_ars_cap(struct ndctl_bus *bus, u64 start, u64 size)
> +{
> + int rc;
> +
> + clear_err.ars_cap = ndctl_bus_cmd_new_ars_cap(bus, start, size);
> + if (!clear_err.ars_cap) {
> + error("%s: bus: %s failed to create cmd\n",
> + __func__, ndctl_bus_get_provider(bus));
> + return -ENOTTY;
> + }
> +
> + rc = ndctl_cmd_submit(clear_err.ars_cap);
> + if (rc) {
> + error("%s: bus: %s failed to submit cmd: %d\n",
> + __func__, ndctl_bus_get_provider(bus), rc);
> + ndctl_cmd_unref(clear_err.ars_cap);
> + return rc;
> + }
> +
> + if (ndctl_cmd_ars_cap_get_size(clear_err.ars_cap) <
> + sizeof(struct nd_cmd_ars_status)){
> + error("%s: bus: %s expected size >= %zd got: %d\n",
> + __func__, ndctl_bus_get_provider(bus),
> + sizeof(struct nd_cmd_ars_status),
> +
> ndctl_cmd_ars_cap_get_size(clear_err.ars_cap));
> + ndctl_cmd_unref(clear_err.ars_cap);
> + return -ENXIO;
> + }
> +
> + return 0;
> +}
> +
> +static int match_dev(struct clear_err *ce, char *dev_name)
> +{
> + ndctl_bus_foreach(ce->ctx, ce->bus)
> + ndctl_region_foreach(ce->bus, ce->region)
> + ndctl_dax_foreach(ce->region, ce->dax)
> + if (strncmp(basename(dev_name),
> + ndctl_dax_get_devname(ce->dax), 256)
This device is an nvdimm nd_dax device, not a device-dax character
device instance. See calls to util_daxctl_region_to_json() and the
implementation of that routine for how to lookup the device-dax
character device from the ndctl_dax instance returned by
ndctl_dax_foreach().
I don't like that this routine is silently trampling on ce->dax. If it
finds the proper dax instance it should return it directly or NULL
otherwise.
Lastly you'll need to match by actual major:minor number otherwise how
do you know that an argument of /dev/my-special-device-dax-symlink
refers to the device-dax instance you're attempting to match?
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
