* [PATCH 1/2 v2] UBI: Add initial support for scatter gather
@ 2015-01-10 21:52 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-10 21:52 UTC (permalink / raw)
To: dedekind1
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, Richard Weinberger
Adds a new set of functions to deal with scatter gather.
ubi_eba_read_leb_sg() will read from a LEB into a scatter gather list.
The new data structure struct ubi_sgl will be used within UBI to
hold the scatter gather list itself and metadata to have a cursor
within the list.
Signed-off-by: Richard Weinberger <richard@nod.at>
Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
drivers/mtd/ubi/eba.c | 56 +++++++++++++++++++++++++++++
drivers/mtd/ubi/kapi.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
drivers/mtd/ubi/ubi.h | 3 ++
include/linux/mtd/ubi.h | 46 ++++++++++++++++++++++++
4 files changed, 185 insertions(+), 15 deletions(-)
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index b698534..0aaf707 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -481,6 +481,62 @@ out_unlock:
}
/**
+ * ubi_eba_read_leb_sg - read data into a scatter gather list.
+ * @ubi: UBI device description object
+ * @vol: volume description object
+ * @lnum: logical eraseblock number
+ * @sgl: UBI scatter gather list to store the read data
+ * @offset: offset from where to read
+ * @len: how many bytes to read
+ * @check: data CRC check flag
+ *
+ * This function works exactly like ubi_eba_read_leb(). But instead of
+ * storing the read data into a buffer it writes to an UBI scatter gather
+ * list.
+ */
+int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
+ struct ubi_sgl *sgl, int lnum, int offset, int len,
+ int check)
+{
+ int to_read;
+ int ret;
+ struct scatterlist *sg;
+
+ for (;;) {
+ ubi_assert(sgl->list_pos < UBI_MAX_SG_COUNT);
+ sg = &sgl->sg[sgl->list_pos];
+ if (len < sg->length - sgl->page_pos)
+ to_read = len;
+ else
+ to_read = sg->length - sgl->page_pos;
+
+ ret = ubi_eba_read_leb(ubi, vol, lnum,
+ sg_virt(sg) + sgl->page_pos, offset,
+ to_read, check);
+ if (ret < 0)
+ goto out;
+
+ offset += to_read;
+ len -= to_read;
+ if (!len) {
+ sgl->page_pos += to_read;
+ if (sgl->page_pos == sg->length) {
+ sgl->list_pos++;
+ sgl->page_pos = 0;
+ }
+
+ break;
+ }
+
+ sgl->list_pos++;
+ sgl->page_pos = 0;
+ }
+
+out:
+ return ret;
+}
+
+/**
* recover_peb - recover from write failure.
* @ubi: UBI device description object
* @pnum: the physical eraseblock to recover
diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
index f3bab66..d0055a2 100644
--- a/drivers/mtd/ubi/kapi.c
+++ b/drivers/mtd/ubi/kapi.c
@@ -355,6 +355,43 @@ void ubi_close_volume(struct ubi_volume_desc *desc)
EXPORT_SYMBOL_GPL(ubi_close_volume);
/**
+ * leb_read_sanity_check - does sanity checks on read requests.
+ * @desc: volume descriptor
+ * @lnum: logical eraseblock number to read from
+ * @offset: offset within the logical eraseblock to read from
+ * @len: how many bytes to read
+ *
+ * This function is used by ubi_leb_read() and ubi_leb_read_sg()
+ * to perfrom sanity checks.
+ */
+static int leb_read_sanity_check(struct ubi_volume_desc *desc, int lnum,
+ int offset, int len)
+{
+ struct ubi_volume *vol = desc->vol;
+ struct ubi_device *ubi = vol->ubi;
+ int vol_id = vol->vol_id;
+
+ if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
+ lnum >= vol->used_ebs || offset < 0 || len < 0 ||
+ offset + len > vol->usable_leb_size)
+ return -EINVAL;
+
+ if (vol->vol_type == UBI_STATIC_VOLUME) {
+ if (vol->used_ebs == 0)
+ /* Empty static UBI volume */
+ return 0;
+ if (lnum == vol->used_ebs - 1 &&
+ offset + len > vol->last_eb_bytes)
+ return -EINVAL;
+ }
+
+ if (vol->upd_marker)
+ return -EBADF;
+
+ return 0;
+}
+
+/**
* ubi_leb_read - read data.
* @desc: volume descriptor
* @lnum: logical eraseblock number to read from
@@ -390,22 +427,10 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
- if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
- lnum >= vol->used_ebs || offset < 0 || len < 0 ||
- offset + len > vol->usable_leb_size)
- return -EINVAL;
-
- if (vol->vol_type == UBI_STATIC_VOLUME) {
- if (vol->used_ebs == 0)
- /* Empty static UBI volume */
- return 0;
- if (lnum == vol->used_ebs - 1 &&
- offset + len > vol->last_eb_bytes)
- return -EINVAL;
- }
+ err = leb_read_sanity_check(desc, lnum, offset, len);
+ if (err < 0)
+ return err;
- if (vol->upd_marker)
- return -EBADF;
if (len == 0)
return 0;
@@ -419,6 +444,46 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
}
EXPORT_SYMBOL_GPL(ubi_leb_read);
+
+/**
+ * ubi_leb_read_sg - read data into a scatter gather list.
+ * @desc: volume descriptor
+ * @lnum: logical eraseblock number to read from
+ * @buf: buffer where to store the read data
+ * @offset: offset within the logical eraseblock to read from
+ * @len: how many bytes to read
+ * @check: whether UBI has to check the read data's CRC or not.
+ *
+ * This function works exactly like ubi_leb_read_sg(). But instead of
+ * storing the read data into a buffer it writes to an UBI scatter gather
+ * list.
+ */
+int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
+ int offset, int len, int check)
+{
+ struct ubi_volume *vol = desc->vol;
+ struct ubi_device *ubi = vol->ubi;
+ int err, vol_id = vol->vol_id;
+
+ dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
+
+ err = leb_read_sanity_check(desc, lnum, offset, len);
+ if (err < 0)
+ return err;
+
+ if (len == 0)
+ return 0;
+
+ err = ubi_eba_read_leb_sg(ubi, vol, sgl, lnum, offset, len, check);
+ if (err && mtd_is_eccerr(err) && vol->vol_type == UBI_STATIC_VOLUME) {
+ ubi_warn(ubi, "mark volume %d as corrupted", vol_id);
+ vol->corrupted = 1;
+ }
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(ubi_leb_read_sg);
+
/**
* ubi_leb_write - write data.
* @desc: volume descriptor
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index ee7ac0b..a5283cf 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -791,6 +791,9 @@ int ubi_eba_unmap_leb(struct ubi_device *ubi, struct ubi_volume *vol,
int lnum);
int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
void *buf, int offset, int len, int check);
+int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
+ struct ubi_sgl *sgl, int lnum, int offset, int len,
+ int check);
int ubi_eba_write_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
const void *buf, int offset, int len);
int ubi_eba_write_leb_st(struct ubi_device *ubi, struct ubi_volume *vol,
diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
index c3918a0..3d5916d 100644
--- a/include/linux/mtd/ubi.h
+++ b/include/linux/mtd/ubi.h
@@ -23,11 +23,16 @@
#include <linux/ioctl.h>
#include <linux/types.h>
+#include <linux/scatterlist.h>
#include <mtd/ubi-user.h>
/* All voumes/LEBs */
#define UBI_ALL -1
+/* Maximum number of scatter gather list entries,
+ * we use only 64 to have a lower memory foot print. */
+#define UBI_MAX_SG_COUNT 64
+
/*
* enum ubi_open_mode - UBI volume open mode constants.
*
@@ -116,6 +121,35 @@ struct ubi_volume_info {
};
/**
+ * struct ubi_sgl - UBI scatter gather list data structure.
+ * @list_pos: current position in @sg[]
+ * @page_pos: current position in @sg[@list_pos]
+ * @sg: the scatter gather list itself
+ *
+ * ubi_sgl is a wrapper around a scatter list which keeps track of the
+ * current position in the list and the current list item such that
+ * it can be used across multiple ubi_leb_read_sg() calls.
+ */
+struct ubi_sgl {
+ int list_pos;
+ int page_pos;
+ struct scatterlist sg[UBI_MAX_SG_COUNT];
+};
+
+/**
+ * ubi_sgl_init - initialize an UBI scatter gather list data structure.
+ * @usgl: the UBI scatter gather struct itself
+ *
+ * Please note that you still have to use sg_init_table() or any adequate
+ * function to initialize the unterlaying struct scatterlist.
+ */
+static inline void ubi_sgl_init(struct ubi_sgl *usgl)
+{
+ usgl->list_pos = 0;
+ usgl->page_pos = 0;
+}
+
+/**
* struct ubi_device_info - UBI device description data structure.
* @ubi_num: ubi device number
* @leb_size: logical eraseblock size on this UBI device
@@ -210,6 +244,8 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb);
void ubi_close_volume(struct ubi_volume_desc *desc);
int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
int len, int check);
+int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
+ int offset, int len, int check);
int ubi_leb_write(struct ubi_volume_desc *desc, int lnum, const void *buf,
int offset, int len);
int ubi_leb_change(struct ubi_volume_desc *desc, int lnum, const void *buf,
@@ -230,4 +266,14 @@ static inline int ubi_read(struct ubi_volume_desc *desc, int lnum, char *buf,
{
return ubi_leb_read(desc, lnum, buf, offset, len, 0);
}
+
+/*
+ * This function is the same as the 'ubi_leb_read_sg()' function, but it does
+ * not provide the checking capability.
+ */
+static inline int ubi_read_sg(struct ubi_volume_desc *desc, int lnum,
+ struct ubi_sgl *sgl, int offset, int len)
+{
+ return ubi_leb_read_sg(desc, lnum, sgl, offset, len, 0);
+}
#endif /* !__LINUX_UBI_H__ */
--
1.8.4.5
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 1/2 v2] UBI: Add initial support for scatter gather
@ 2015-01-10 21:52 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-10 21:52 UTC (permalink / raw)
To: dedekind1
Cc: linux-mtd, computersforpeace, dwmw2, linux-kernel, Richard Weinberger
Adds a new set of functions to deal with scatter gather.
ubi_eba_read_leb_sg() will read from a LEB into a scatter gather list.
The new data structure struct ubi_sgl will be used within UBI to
hold the scatter gather list itself and metadata to have a cursor
within the list.
Signed-off-by: Richard Weinberger <richard@nod.at>
Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
drivers/mtd/ubi/eba.c | 56 +++++++++++++++++++++++++++++
drivers/mtd/ubi/kapi.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
drivers/mtd/ubi/ubi.h | 3 ++
include/linux/mtd/ubi.h | 46 ++++++++++++++++++++++++
4 files changed, 185 insertions(+), 15 deletions(-)
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index b698534..0aaf707 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -481,6 +481,62 @@ out_unlock:
}
/**
+ * ubi_eba_read_leb_sg - read data into a scatter gather list.
+ * @ubi: UBI device description object
+ * @vol: volume description object
+ * @lnum: logical eraseblock number
+ * @sgl: UBI scatter gather list to store the read data
+ * @offset: offset from where to read
+ * @len: how many bytes to read
+ * @check: data CRC check flag
+ *
+ * This function works exactly like ubi_eba_read_leb(). But instead of
+ * storing the read data into a buffer it writes to an UBI scatter gather
+ * list.
+ */
+int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
+ struct ubi_sgl *sgl, int lnum, int offset, int len,
+ int check)
+{
+ int to_read;
+ int ret;
+ struct scatterlist *sg;
+
+ for (;;) {
+ ubi_assert(sgl->list_pos < UBI_MAX_SG_COUNT);
+ sg = &sgl->sg[sgl->list_pos];
+ if (len < sg->length - sgl->page_pos)
+ to_read = len;
+ else
+ to_read = sg->length - sgl->page_pos;
+
+ ret = ubi_eba_read_leb(ubi, vol, lnum,
+ sg_virt(sg) + sgl->page_pos, offset,
+ to_read, check);
+ if (ret < 0)
+ goto out;
+
+ offset += to_read;
+ len -= to_read;
+ if (!len) {
+ sgl->page_pos += to_read;
+ if (sgl->page_pos == sg->length) {
+ sgl->list_pos++;
+ sgl->page_pos = 0;
+ }
+
+ break;
+ }
+
+ sgl->list_pos++;
+ sgl->page_pos = 0;
+ }
+
+out:
+ return ret;
+}
+
+/**
* recover_peb - recover from write failure.
* @ubi: UBI device description object
* @pnum: the physical eraseblock to recover
diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
index f3bab66..d0055a2 100644
--- a/drivers/mtd/ubi/kapi.c
+++ b/drivers/mtd/ubi/kapi.c
@@ -355,6 +355,43 @@ void ubi_close_volume(struct ubi_volume_desc *desc)
EXPORT_SYMBOL_GPL(ubi_close_volume);
/**
+ * leb_read_sanity_check - does sanity checks on read requests.
+ * @desc: volume descriptor
+ * @lnum: logical eraseblock number to read from
+ * @offset: offset within the logical eraseblock to read from
+ * @len: how many bytes to read
+ *
+ * This function is used by ubi_leb_read() and ubi_leb_read_sg()
+ * to perfrom sanity checks.
+ */
+static int leb_read_sanity_check(struct ubi_volume_desc *desc, int lnum,
+ int offset, int len)
+{
+ struct ubi_volume *vol = desc->vol;
+ struct ubi_device *ubi = vol->ubi;
+ int vol_id = vol->vol_id;
+
+ if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
+ lnum >= vol->used_ebs || offset < 0 || len < 0 ||
+ offset + len > vol->usable_leb_size)
+ return -EINVAL;
+
+ if (vol->vol_type == UBI_STATIC_VOLUME) {
+ if (vol->used_ebs == 0)
+ /* Empty static UBI volume */
+ return 0;
+ if (lnum == vol->used_ebs - 1 &&
+ offset + len > vol->last_eb_bytes)
+ return -EINVAL;
+ }
+
+ if (vol->upd_marker)
+ return -EBADF;
+
+ return 0;
+}
+
+/**
* ubi_leb_read - read data.
* @desc: volume descriptor
* @lnum: logical eraseblock number to read from
@@ -390,22 +427,10 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
- if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
- lnum >= vol->used_ebs || offset < 0 || len < 0 ||
- offset + len > vol->usable_leb_size)
- return -EINVAL;
-
- if (vol->vol_type == UBI_STATIC_VOLUME) {
- if (vol->used_ebs == 0)
- /* Empty static UBI volume */
- return 0;
- if (lnum == vol->used_ebs - 1 &&
- offset + len > vol->last_eb_bytes)
- return -EINVAL;
- }
+ err = leb_read_sanity_check(desc, lnum, offset, len);
+ if (err < 0)
+ return err;
- if (vol->upd_marker)
- return -EBADF;
if (len == 0)
return 0;
@@ -419,6 +444,46 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
}
EXPORT_SYMBOL_GPL(ubi_leb_read);
+
+/**
+ * ubi_leb_read_sg - read data into a scatter gather list.
+ * @desc: volume descriptor
+ * @lnum: logical eraseblock number to read from
+ * @buf: buffer where to store the read data
+ * @offset: offset within the logical eraseblock to read from
+ * @len: how many bytes to read
+ * @check: whether UBI has to check the read data's CRC or not.
+ *
+ * This function works exactly like ubi_leb_read_sg(). But instead of
+ * storing the read data into a buffer it writes to an UBI scatter gather
+ * list.
+ */
+int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
+ int offset, int len, int check)
+{
+ struct ubi_volume *vol = desc->vol;
+ struct ubi_device *ubi = vol->ubi;
+ int err, vol_id = vol->vol_id;
+
+ dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
+
+ err = leb_read_sanity_check(desc, lnum, offset, len);
+ if (err < 0)
+ return err;
+
+ if (len == 0)
+ return 0;
+
+ err = ubi_eba_read_leb_sg(ubi, vol, sgl, lnum, offset, len, check);
+ if (err && mtd_is_eccerr(err) && vol->vol_type == UBI_STATIC_VOLUME) {
+ ubi_warn(ubi, "mark volume %d as corrupted", vol_id);
+ vol->corrupted = 1;
+ }
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(ubi_leb_read_sg);
+
/**
* ubi_leb_write - write data.
* @desc: volume descriptor
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index ee7ac0b..a5283cf 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -791,6 +791,9 @@ int ubi_eba_unmap_leb(struct ubi_device *ubi, struct ubi_volume *vol,
int lnum);
int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
void *buf, int offset, int len, int check);
+int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
+ struct ubi_sgl *sgl, int lnum, int offset, int len,
+ int check);
int ubi_eba_write_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
const void *buf, int offset, int len);
int ubi_eba_write_leb_st(struct ubi_device *ubi, struct ubi_volume *vol,
diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
index c3918a0..3d5916d 100644
--- a/include/linux/mtd/ubi.h
+++ b/include/linux/mtd/ubi.h
@@ -23,11 +23,16 @@
#include <linux/ioctl.h>
#include <linux/types.h>
+#include <linux/scatterlist.h>
#include <mtd/ubi-user.h>
/* All voumes/LEBs */
#define UBI_ALL -1
+/* Maximum number of scatter gather list entries,
+ * we use only 64 to have a lower memory foot print. */
+#define UBI_MAX_SG_COUNT 64
+
/*
* enum ubi_open_mode - UBI volume open mode constants.
*
@@ -116,6 +121,35 @@ struct ubi_volume_info {
};
/**
+ * struct ubi_sgl - UBI scatter gather list data structure.
+ * @list_pos: current position in @sg[]
+ * @page_pos: current position in @sg[@list_pos]
+ * @sg: the scatter gather list itself
+ *
+ * ubi_sgl is a wrapper around a scatter list which keeps track of the
+ * current position in the list and the current list item such that
+ * it can be used across multiple ubi_leb_read_sg() calls.
+ */
+struct ubi_sgl {
+ int list_pos;
+ int page_pos;
+ struct scatterlist sg[UBI_MAX_SG_COUNT];
+};
+
+/**
+ * ubi_sgl_init - initialize an UBI scatter gather list data structure.
+ * @usgl: the UBI scatter gather struct itself
+ *
+ * Please note that you still have to use sg_init_table() or any adequate
+ * function to initialize the unterlaying struct scatterlist.
+ */
+static inline void ubi_sgl_init(struct ubi_sgl *usgl)
+{
+ usgl->list_pos = 0;
+ usgl->page_pos = 0;
+}
+
+/**
* struct ubi_device_info - UBI device description data structure.
* @ubi_num: ubi device number
* @leb_size: logical eraseblock size on this UBI device
@@ -210,6 +244,8 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb);
void ubi_close_volume(struct ubi_volume_desc *desc);
int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
int len, int check);
+int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
+ int offset, int len, int check);
int ubi_leb_write(struct ubi_volume_desc *desc, int lnum, const void *buf,
int offset, int len);
int ubi_leb_change(struct ubi_volume_desc *desc, int lnum, const void *buf,
@@ -230,4 +266,14 @@ static inline int ubi_read(struct ubi_volume_desc *desc, int lnum, char *buf,
{
return ubi_leb_read(desc, lnum, buf, offset, len, 0);
}
+
+/*
+ * This function is the same as the 'ubi_leb_read_sg()' function, but it does
+ * not provide the checking capability.
+ */
+static inline int ubi_read_sg(struct ubi_volume_desc *desc, int lnum,
+ struct ubi_sgl *sgl, int offset, int len)
+{
+ return ubi_leb_read_sg(desc, lnum, sgl, offset, len, 0);
+}
#endif /* !__LINUX_UBI_H__ */
--
1.8.4.5
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-10 21:52 ` Richard Weinberger
@ 2015-01-10 21:52 ` Richard Weinberger
-1 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-10 21:52 UTC (permalink / raw)
To: dedekind1
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel,
Richard Weinberger, hch, axboe, tom.leiming
Convert the driver to blk-mq.
Beside of moving to the modern block interface this change boosts
also the performance of the driver.
nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
nand: Micron NAND 256MiB 3,3V 8-bit
nand: 256MiB, SLC, page size: 2048, OOB size: 64
root@debian-armhf:~# dd if=/dev/ubiblock0_0 of=/dev/zero bs=1M
243+1 records in
243+1 records out
255080448 bytes (255 MB) copied, 4.39295 s, 58.1 MB/s
vs.
root@debian-armhf:~# dd if=/dev/ubiblock0_0 of=/dev/zero bs=1M
243+1 records in
243+1 records out
255080448 bytes (255 MB) copied, 2.87676 s, 88.7 MB/s
Cc: hch@infradead.org
Cc: axboe@fb.com
Cc: tom.leiming@gmail.com
Signed-off-by: Richard Weinberger <richard@nod.at>
Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
drivers/mtd/ubi/block.c | 202 ++++++++++++++++++++++--------------------------
1 file changed, 94 insertions(+), 108 deletions(-)
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 6b6bce2..00caf46 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -42,11 +42,12 @@
#include <linux/list.h>
#include <linux/mutex.h>
#include <linux/slab.h>
-#include <linux/vmalloc.h>
#include <linux/mtd/ubi.h>
#include <linux/workqueue.h>
#include <linux/blkdev.h>
+#include <linux/blk-mq.h>
#include <linux/hdreg.h>
+#include <linux/scatterlist.h>
#include <asm/div64.h>
#include "ubi-media.h"
@@ -67,6 +68,11 @@ struct ubiblock_param {
char name[UBIBLOCK_PARAM_LEN+1];
};
+struct ubiblock_pdu {
+ struct work_struct work;
+ struct ubi_sgl usgl;
+};
+
/* Numbers of elements set in the @ubiblock_param array */
static int ubiblock_devs __initdata;
@@ -84,11 +90,10 @@ struct ubiblock {
struct request_queue *rq;
struct workqueue_struct *wq;
- struct work_struct work;
struct mutex dev_mutex;
- spinlock_t queue_lock;
struct list_head list;
+ struct blk_mq_tag_set tag_set;
};
/* Linked list of all ubiblock instances */
@@ -181,31 +186,20 @@ static struct ubiblock *find_dev_nolock(int ubi_num, int vol_id)
return NULL;
}
-static int ubiblock_read_to_buf(struct ubiblock *dev, char *buffer,
- int leb, int offset, int len)
+static int ubiblock_read(struct ubiblock_pdu *pdu)
{
- int ret;
+ int ret, leb, offset, bytes_left, to_read;
+ u64 pos;
+ struct request *req = blk_mq_rq_from_pdu(pdu);
+ struct ubiblock *dev = req->q->queuedata;
- ret = ubi_read(dev->desc, leb, buffer, offset, len);
- if (ret) {
- dev_err(disk_to_dev(dev->gd), "%d while reading from LEB %d (offset %d, length %d)",
- ret, leb, offset, len);
- return ret;
- }
- return 0;
-}
-
-static int ubiblock_read(struct ubiblock *dev, char *buffer,
- sector_t sec, int len)
-{
- int ret, leb, offset;
- int bytes_left = len;
- int to_read = len;
- u64 pos = sec << 9;
+ to_read = blk_rq_bytes(req);
+ pos = blk_rq_pos(req) << 9;
/* Get LEB:offset address to read from */
offset = do_div(pos, dev->leb_size);
leb = pos;
+ bytes_left = to_read;
while (bytes_left) {
/*
@@ -215,11 +209,10 @@ static int ubiblock_read(struct ubiblock *dev, char *buffer,
if (offset + to_read > dev->leb_size)
to_read = dev->leb_size - offset;
- ret = ubiblock_read_to_buf(dev, buffer, leb, offset, to_read);
- if (ret)
+ ret = ubi_read_sg(dev->desc, leb, &pdu->usgl, offset, to_read);
+ if (ret < 0)
return ret;
- buffer += to_read;
bytes_left -= to_read;
to_read = bytes_left;
leb += 1;
@@ -228,79 +221,6 @@ static int ubiblock_read(struct ubiblock *dev, char *buffer,
return 0;
}
-static int do_ubiblock_request(struct ubiblock *dev, struct request *req)
-{
- int len, ret;
- sector_t sec;
-
- if (req->cmd_type != REQ_TYPE_FS)
- return -EIO;
-
- if (blk_rq_pos(req) + blk_rq_cur_sectors(req) >
- get_capacity(req->rq_disk))
- return -EIO;
-
- if (rq_data_dir(req) != READ)
- return -ENOSYS; /* Write not implemented */
-
- sec = blk_rq_pos(req);
- len = blk_rq_cur_bytes(req);
-
- /*
- * Let's prevent the device from being removed while we're doing I/O
- * work. Notice that this means we serialize all the I/O operations,
- * but it's probably of no impact given the NAND core serializes
- * flash access anyway.
- */
- mutex_lock(&dev->dev_mutex);
- ret = ubiblock_read(dev, bio_data(req->bio), sec, len);
- mutex_unlock(&dev->dev_mutex);
-
- return ret;
-}
-
-static void ubiblock_do_work(struct work_struct *work)
-{
- struct ubiblock *dev =
- container_of(work, struct ubiblock, work);
- struct request_queue *rq = dev->rq;
- struct request *req;
- int res;
-
- spin_lock_irq(rq->queue_lock);
-
- req = blk_fetch_request(rq);
- while (req) {
-
- spin_unlock_irq(rq->queue_lock);
- res = do_ubiblock_request(dev, req);
- spin_lock_irq(rq->queue_lock);
-
- /*
- * If we're done with this request,
- * we need to fetch a new one
- */
- if (!__blk_end_request_cur(req, res))
- req = blk_fetch_request(rq);
- }
-
- spin_unlock_irq(rq->queue_lock);
-}
-
-static void ubiblock_request(struct request_queue *rq)
-{
- struct ubiblock *dev;
- struct request *req;
-
- dev = rq->queuedata;
-
- if (!dev)
- while ((req = blk_fetch_request(rq)) != NULL)
- __blk_end_request_all(req, -ENODEV);
- else
- queue_work(dev->wq, &dev->work);
-}
-
static int ubiblock_open(struct block_device *bdev, fmode_t mode)
{
struct ubiblock *dev = bdev->bd_disk->private_data;
@@ -374,6 +294,57 @@ static const struct block_device_operations ubiblock_ops = {
.getgeo = ubiblock_getgeo,
};
+static void ubiblock_do_work(struct work_struct *work)
+{
+ int ret;
+ struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
+ struct request *req = blk_mq_rq_from_pdu(pdu);
+
+ blk_mq_start_request(req);
+ blk_rq_map_sg(req->q, req, pdu->usgl.sg);
+
+ ret = ubiblock_read(pdu);
+ blk_mq_end_request(req, ret);
+}
+
+static int ubiblock_queue_rq(struct blk_mq_hw_ctx *hctx,
+ const struct blk_mq_queue_data *bd)
+{
+ struct request *req = bd->rq;
+ struct ubiblock *dev = hctx->queue->queuedata;
+ struct ubiblock_pdu *pdu = blk_mq_rq_to_pdu(req);
+
+ if (req->cmd_type != REQ_TYPE_FS)
+ return BLK_MQ_RQ_QUEUE_ERROR;
+
+ if (rq_data_dir(req) != READ)
+ return BLK_MQ_RQ_QUEUE_ERROR; /* Write not implemented */
+
+ ubi_sgl_init(&pdu->usgl);
+ queue_work(dev->wq, &pdu->work);
+
+ return BLK_MQ_RQ_QUEUE_OK;
+}
+
+static int ubiblock_init_request(void *data, struct request *req,
+ unsigned int hctx_idx,
+ unsigned int request_idx,
+ unsigned int numa_node)
+{
+ struct ubiblock_pdu *pdu = blk_mq_rq_to_pdu(req);
+
+ sg_init_table(pdu->usgl.sg, UBI_MAX_SG_COUNT);
+ INIT_WORK(&pdu->work, ubiblock_do_work);
+
+ return 0;
+}
+
+static struct blk_mq_ops ubiblock_mq_ops = {
+ .queue_rq = ubiblock_queue_rq,
+ .init_request = ubiblock_init_request,
+ .map_queue = blk_mq_map_queue,
+};
+
int ubiblock_create(struct ubi_volume_info *vi)
{
struct ubiblock *dev;
@@ -417,13 +388,27 @@ int ubiblock_create(struct ubi_volume_info *vi)
set_capacity(gd, disk_capacity);
dev->gd = gd;
- spin_lock_init(&dev->queue_lock);
- dev->rq = blk_init_queue(ubiblock_request, &dev->queue_lock);
+ dev->tag_set.ops = &ubiblock_mq_ops;
+ dev->tag_set.queue_depth = 64;
+ dev->tag_set.numa_node = NUMA_NO_NODE;
+ dev->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
+ dev->tag_set.cmd_size = sizeof(struct ubiblock_pdu);
+ dev->tag_set.driver_data = dev;
+ dev->tag_set.nr_hw_queues = 1;
+
+ ret = blk_mq_alloc_tag_set(&dev->tag_set);
+ if (ret) {
+ dev_err(disk_to_dev(dev->gd), "blk_mq_alloc_tag_set failed");
+ goto out_put_disk;
+ }
+
+ dev->rq = blk_mq_init_queue(&dev->tag_set);
if (!dev->rq) {
- dev_err(disk_to_dev(gd), "blk_init_queue failed");
+ dev_err(disk_to_dev(gd), "blk_mq_init_queue failed");
ret = -ENODEV;
- goto out_put_disk;
+ goto out_free_tags;
}
+ blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
dev->rq->queuedata = dev;
dev->gd->queue = dev->rq;
@@ -437,7 +422,6 @@ int ubiblock_create(struct ubi_volume_info *vi)
ret = -ENOMEM;
goto out_free_queue;
}
- INIT_WORK(&dev->work, ubiblock_do_work);
mutex_lock(&devices_mutex);
list_add_tail(&dev->list, &ubiblock_devices);
@@ -451,6 +435,8 @@ int ubiblock_create(struct ubi_volume_info *vi)
out_free_queue:
blk_cleanup_queue(dev->rq);
+out_free_tags:
+ blk_mq_free_tag_set(&dev->tag_set);
out_put_disk:
put_disk(dev->gd);
out_free_dev:
@@ -461,8 +447,13 @@ out_free_dev:
static void ubiblock_cleanup(struct ubiblock *dev)
{
+ /* Stop new requests to arrive */
del_gendisk(dev->gd);
+ /* Flush pending work */
+ destroy_workqueue(dev->wq);
+ /* Finally destroy the blk queue */
blk_cleanup_queue(dev->rq);
+ blk_mq_free_tag_set(&dev->tag_set);
dev_info(disk_to_dev(dev->gd), "released");
put_disk(dev->gd);
}
@@ -490,9 +481,6 @@ int ubiblock_remove(struct ubi_volume_info *vi)
list_del(&dev->list);
mutex_unlock(&devices_mutex);
- /* Flush pending work and stop this workqueue */
- destroy_workqueue(dev->wq);
-
ubiblock_cleanup(dev);
mutex_unlock(&dev->dev_mutex);
kfree(dev);
@@ -620,8 +608,6 @@ static void ubiblock_remove_all(void)
struct ubiblock *dev;
list_for_each_entry_safe(dev, next, &ubiblock_devices, list) {
- /* Flush pending work and stop workqueue */
- destroy_workqueue(dev->wq);
/* The module is being forcefully removed */
WARN_ON(dev->desc);
/* Remove from device list */
--
1.8.4.5
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-10 21:52 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-10 21:52 UTC (permalink / raw)
To: dedekind1
Cc: axboe, Richard Weinberger, linux-kernel, hch, linux-mtd,
tom.leiming, computersforpeace, dwmw2
Convert the driver to blk-mq.
Beside of moving to the modern block interface this change boosts
also the performance of the driver.
nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
nand: Micron NAND 256MiB 3,3V 8-bit
nand: 256MiB, SLC, page size: 2048, OOB size: 64
root@debian-armhf:~# dd if=/dev/ubiblock0_0 of=/dev/zero bs=1M
243+1 records in
243+1 records out
255080448 bytes (255 MB) copied, 4.39295 s, 58.1 MB/s
vs.
root@debian-armhf:~# dd if=/dev/ubiblock0_0 of=/dev/zero bs=1M
243+1 records in
243+1 records out
255080448 bytes (255 MB) copied, 2.87676 s, 88.7 MB/s
Cc: hch@infradead.org
Cc: axboe@fb.com
Cc: tom.leiming@gmail.com
Signed-off-by: Richard Weinberger <richard@nod.at>
Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
drivers/mtd/ubi/block.c | 202 ++++++++++++++++++++++--------------------------
1 file changed, 94 insertions(+), 108 deletions(-)
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 6b6bce2..00caf46 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -42,11 +42,12 @@
#include <linux/list.h>
#include <linux/mutex.h>
#include <linux/slab.h>
-#include <linux/vmalloc.h>
#include <linux/mtd/ubi.h>
#include <linux/workqueue.h>
#include <linux/blkdev.h>
+#include <linux/blk-mq.h>
#include <linux/hdreg.h>
+#include <linux/scatterlist.h>
#include <asm/div64.h>
#include "ubi-media.h"
@@ -67,6 +68,11 @@ struct ubiblock_param {
char name[UBIBLOCK_PARAM_LEN+1];
};
+struct ubiblock_pdu {
+ struct work_struct work;
+ struct ubi_sgl usgl;
+};
+
/* Numbers of elements set in the @ubiblock_param array */
static int ubiblock_devs __initdata;
@@ -84,11 +90,10 @@ struct ubiblock {
struct request_queue *rq;
struct workqueue_struct *wq;
- struct work_struct work;
struct mutex dev_mutex;
- spinlock_t queue_lock;
struct list_head list;
+ struct blk_mq_tag_set tag_set;
};
/* Linked list of all ubiblock instances */
@@ -181,31 +186,20 @@ static struct ubiblock *find_dev_nolock(int ubi_num, int vol_id)
return NULL;
}
-static int ubiblock_read_to_buf(struct ubiblock *dev, char *buffer,
- int leb, int offset, int len)
+static int ubiblock_read(struct ubiblock_pdu *pdu)
{
- int ret;
+ int ret, leb, offset, bytes_left, to_read;
+ u64 pos;
+ struct request *req = blk_mq_rq_from_pdu(pdu);
+ struct ubiblock *dev = req->q->queuedata;
- ret = ubi_read(dev->desc, leb, buffer, offset, len);
- if (ret) {
- dev_err(disk_to_dev(dev->gd), "%d while reading from LEB %d (offset %d, length %d)",
- ret, leb, offset, len);
- return ret;
- }
- return 0;
-}
-
-static int ubiblock_read(struct ubiblock *dev, char *buffer,
- sector_t sec, int len)
-{
- int ret, leb, offset;
- int bytes_left = len;
- int to_read = len;
- u64 pos = sec << 9;
+ to_read = blk_rq_bytes(req);
+ pos = blk_rq_pos(req) << 9;
/* Get LEB:offset address to read from */
offset = do_div(pos, dev->leb_size);
leb = pos;
+ bytes_left = to_read;
while (bytes_left) {
/*
@@ -215,11 +209,10 @@ static int ubiblock_read(struct ubiblock *dev, char *buffer,
if (offset + to_read > dev->leb_size)
to_read = dev->leb_size - offset;
- ret = ubiblock_read_to_buf(dev, buffer, leb, offset, to_read);
- if (ret)
+ ret = ubi_read_sg(dev->desc, leb, &pdu->usgl, offset, to_read);
+ if (ret < 0)
return ret;
- buffer += to_read;
bytes_left -= to_read;
to_read = bytes_left;
leb += 1;
@@ -228,79 +221,6 @@ static int ubiblock_read(struct ubiblock *dev, char *buffer,
return 0;
}
-static int do_ubiblock_request(struct ubiblock *dev, struct request *req)
-{
- int len, ret;
- sector_t sec;
-
- if (req->cmd_type != REQ_TYPE_FS)
- return -EIO;
-
- if (blk_rq_pos(req) + blk_rq_cur_sectors(req) >
- get_capacity(req->rq_disk))
- return -EIO;
-
- if (rq_data_dir(req) != READ)
- return -ENOSYS; /* Write not implemented */
-
- sec = blk_rq_pos(req);
- len = blk_rq_cur_bytes(req);
-
- /*
- * Let's prevent the device from being removed while we're doing I/O
- * work. Notice that this means we serialize all the I/O operations,
- * but it's probably of no impact given the NAND core serializes
- * flash access anyway.
- */
- mutex_lock(&dev->dev_mutex);
- ret = ubiblock_read(dev, bio_data(req->bio), sec, len);
- mutex_unlock(&dev->dev_mutex);
-
- return ret;
-}
-
-static void ubiblock_do_work(struct work_struct *work)
-{
- struct ubiblock *dev =
- container_of(work, struct ubiblock, work);
- struct request_queue *rq = dev->rq;
- struct request *req;
- int res;
-
- spin_lock_irq(rq->queue_lock);
-
- req = blk_fetch_request(rq);
- while (req) {
-
- spin_unlock_irq(rq->queue_lock);
- res = do_ubiblock_request(dev, req);
- spin_lock_irq(rq->queue_lock);
-
- /*
- * If we're done with this request,
- * we need to fetch a new one
- */
- if (!__blk_end_request_cur(req, res))
- req = blk_fetch_request(rq);
- }
-
- spin_unlock_irq(rq->queue_lock);
-}
-
-static void ubiblock_request(struct request_queue *rq)
-{
- struct ubiblock *dev;
- struct request *req;
-
- dev = rq->queuedata;
-
- if (!dev)
- while ((req = blk_fetch_request(rq)) != NULL)
- __blk_end_request_all(req, -ENODEV);
- else
- queue_work(dev->wq, &dev->work);
-}
-
static int ubiblock_open(struct block_device *bdev, fmode_t mode)
{
struct ubiblock *dev = bdev->bd_disk->private_data;
@@ -374,6 +294,57 @@ static const struct block_device_operations ubiblock_ops = {
.getgeo = ubiblock_getgeo,
};
+static void ubiblock_do_work(struct work_struct *work)
+{
+ int ret;
+ struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
+ struct request *req = blk_mq_rq_from_pdu(pdu);
+
+ blk_mq_start_request(req);
+ blk_rq_map_sg(req->q, req, pdu->usgl.sg);
+
+ ret = ubiblock_read(pdu);
+ blk_mq_end_request(req, ret);
+}
+
+static int ubiblock_queue_rq(struct blk_mq_hw_ctx *hctx,
+ const struct blk_mq_queue_data *bd)
+{
+ struct request *req = bd->rq;
+ struct ubiblock *dev = hctx->queue->queuedata;
+ struct ubiblock_pdu *pdu = blk_mq_rq_to_pdu(req);
+
+ if (req->cmd_type != REQ_TYPE_FS)
+ return BLK_MQ_RQ_QUEUE_ERROR;
+
+ if (rq_data_dir(req) != READ)
+ return BLK_MQ_RQ_QUEUE_ERROR; /* Write not implemented */
+
+ ubi_sgl_init(&pdu->usgl);
+ queue_work(dev->wq, &pdu->work);
+
+ return BLK_MQ_RQ_QUEUE_OK;
+}
+
+static int ubiblock_init_request(void *data, struct request *req,
+ unsigned int hctx_idx,
+ unsigned int request_idx,
+ unsigned int numa_node)
+{
+ struct ubiblock_pdu *pdu = blk_mq_rq_to_pdu(req);
+
+ sg_init_table(pdu->usgl.sg, UBI_MAX_SG_COUNT);
+ INIT_WORK(&pdu->work, ubiblock_do_work);
+
+ return 0;
+}
+
+static struct blk_mq_ops ubiblock_mq_ops = {
+ .queue_rq = ubiblock_queue_rq,
+ .init_request = ubiblock_init_request,
+ .map_queue = blk_mq_map_queue,
+};
+
int ubiblock_create(struct ubi_volume_info *vi)
{
struct ubiblock *dev;
@@ -417,13 +388,27 @@ int ubiblock_create(struct ubi_volume_info *vi)
set_capacity(gd, disk_capacity);
dev->gd = gd;
- spin_lock_init(&dev->queue_lock);
- dev->rq = blk_init_queue(ubiblock_request, &dev->queue_lock);
+ dev->tag_set.ops = &ubiblock_mq_ops;
+ dev->tag_set.queue_depth = 64;
+ dev->tag_set.numa_node = NUMA_NO_NODE;
+ dev->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
+ dev->tag_set.cmd_size = sizeof(struct ubiblock_pdu);
+ dev->tag_set.driver_data = dev;
+ dev->tag_set.nr_hw_queues = 1;
+
+ ret = blk_mq_alloc_tag_set(&dev->tag_set);
+ if (ret) {
+ dev_err(disk_to_dev(dev->gd), "blk_mq_alloc_tag_set failed");
+ goto out_put_disk;
+ }
+
+ dev->rq = blk_mq_init_queue(&dev->tag_set);
if (!dev->rq) {
- dev_err(disk_to_dev(gd), "blk_init_queue failed");
+ dev_err(disk_to_dev(gd), "blk_mq_init_queue failed");
ret = -ENODEV;
- goto out_put_disk;
+ goto out_free_tags;
}
+ blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
dev->rq->queuedata = dev;
dev->gd->queue = dev->rq;
@@ -437,7 +422,6 @@ int ubiblock_create(struct ubi_volume_info *vi)
ret = -ENOMEM;
goto out_free_queue;
}
- INIT_WORK(&dev->work, ubiblock_do_work);
mutex_lock(&devices_mutex);
list_add_tail(&dev->list, &ubiblock_devices);
@@ -451,6 +435,8 @@ int ubiblock_create(struct ubi_volume_info *vi)
out_free_queue:
blk_cleanup_queue(dev->rq);
+out_free_tags:
+ blk_mq_free_tag_set(&dev->tag_set);
out_put_disk:
put_disk(dev->gd);
out_free_dev:
@@ -461,8 +447,13 @@ out_free_dev:
static void ubiblock_cleanup(struct ubiblock *dev)
{
+ /* Stop new requests to arrive */
del_gendisk(dev->gd);
+ /* Flush pending work */
+ destroy_workqueue(dev->wq);
+ /* Finally destroy the blk queue */
blk_cleanup_queue(dev->rq);
+ blk_mq_free_tag_set(&dev->tag_set);
dev_info(disk_to_dev(dev->gd), "released");
put_disk(dev->gd);
}
@@ -490,9 +481,6 @@ int ubiblock_remove(struct ubi_volume_info *vi)
list_del(&dev->list);
mutex_unlock(&devices_mutex);
- /* Flush pending work and stop this workqueue */
- destroy_workqueue(dev->wq);
-
ubiblock_cleanup(dev);
mutex_unlock(&dev->dev_mutex);
kfree(dev);
@@ -620,8 +608,6 @@ static void ubiblock_remove_all(void)
struct ubiblock *dev;
list_for_each_entry_safe(dev, next, &ubiblock_devices, list) {
- /* Flush pending work and stop workqueue */
- destroy_workqueue(dev->wq);
/* The module is being forcefully removed */
WARN_ON(dev->desc);
/* Remove from device list */
--
1.8.4.5
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-10 21:52 ` Richard Weinberger
@ 2015-01-13 16:25 ` Christoph Hellwig
-1 siblings, 0 replies; 32+ messages in thread
From: Christoph Hellwig @ 2015-01-13 16:25 UTC (permalink / raw)
To: Richard Weinberger
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
hch, axboe, tom.leiming
> + struct ubi_sgl usgl;
Btw, what's in struct ubi_sgl? Can't find that in my tree.
> +static void ubiblock_do_work(struct work_struct *work)
> +{
> + int ret;
> + struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
> + struct request *req = blk_mq_rq_from_pdu(pdu);
> +
> + blk_mq_start_request(req);
> + blk_rq_map_sg(req->q, req, pdu->usgl.sg);
blk_rq_map_sg returns the number of entries actually mapped, which
might be smaller than the number passed in due to merging.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-13 16:25 ` Christoph Hellwig
0 siblings, 0 replies; 32+ messages in thread
From: Christoph Hellwig @ 2015-01-13 16:25 UTC (permalink / raw)
To: Richard Weinberger
Cc: axboe, dedekind1, tom.leiming, linux-kernel, hch, linux-mtd,
computersforpeace, dwmw2
> + struct ubi_sgl usgl;
Btw, what's in struct ubi_sgl? Can't find that in my tree.
> +static void ubiblock_do_work(struct work_struct *work)
> +{
> + int ret;
> + struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
> + struct request *req = blk_mq_rq_from_pdu(pdu);
> +
> + blk_mq_start_request(req);
> + blk_rq_map_sg(req->q, req, pdu->usgl.sg);
blk_rq_map_sg returns the number of entries actually mapped, which
might be smaller than the number passed in due to merging.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-13 16:25 ` Christoph Hellwig
@ 2015-01-13 22:51 ` Richard Weinberger
-1 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-13 22:51 UTC (permalink / raw)
To: Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
axboe, tom.leiming
Am 13.01.2015 um 17:25 schrieb Christoph Hellwig:
>> + struct ubi_sgl usgl;
>
> Btw, what's in struct ubi_sgl? Can't find that in my tree.
"[PATCH 1/2] UBI: Add initial support for scatter gather" adds it.
/**
* struct ubi_sgl - UBI scatter gather list data structure.
* @list_pos: current position in @sg[]
* @page_pos: current position in @sg[@list_pos]
* @sg: the scatter gather list itself
*
* ubi_sgl is a wrapper around a scatter list which keeps track of the
* current position in the list and the current list item such that
* it can be used across multiple ubi_leb_read_sg() calls.
*/
struct ubi_sgl {
int list_pos;
int page_pos;
struct scatterlist sg[UBI_MAX_SG_COUNT];
};
>> +static void ubiblock_do_work(struct work_struct *work)
>> +{
>> + int ret;
>> + struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
>> + struct request *req = blk_mq_rq_from_pdu(pdu);
>> +
>> + blk_mq_start_request(req);
>> + blk_rq_map_sg(req->q, req, pdu->usgl.sg);
>
> blk_rq_map_sg returns the number of entries actually mapped, which
> might be smaller than the number passed in due to merging.
Yep, but the ubi_sql has a fixed number of scatterlist entries, UBI_MAX_SG_COUNT.
And I limit it also to that using: blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
Is there another reason why I should use the return value of blk_rq_map_sg()?
Please also note that the UBI block driver is read-only.
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-13 22:51 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-13 22:51 UTC (permalink / raw)
To: Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, axboe, linux-mtd,
computersforpeace, dwmw2
Am 13.01.2015 um 17:25 schrieb Christoph Hellwig:
>> + struct ubi_sgl usgl;
>
> Btw, what's in struct ubi_sgl? Can't find that in my tree.
"[PATCH 1/2] UBI: Add initial support for scatter gather" adds it.
/**
* struct ubi_sgl - UBI scatter gather list data structure.
* @list_pos: current position in @sg[]
* @page_pos: current position in @sg[@list_pos]
* @sg: the scatter gather list itself
*
* ubi_sgl is a wrapper around a scatter list which keeps track of the
* current position in the list and the current list item such that
* it can be used across multiple ubi_leb_read_sg() calls.
*/
struct ubi_sgl {
int list_pos;
int page_pos;
struct scatterlist sg[UBI_MAX_SG_COUNT];
};
>> +static void ubiblock_do_work(struct work_struct *work)
>> +{
>> + int ret;
>> + struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
>> + struct request *req = blk_mq_rq_from_pdu(pdu);
>> +
>> + blk_mq_start_request(req);
>> + blk_rq_map_sg(req->q, req, pdu->usgl.sg);
>
> blk_rq_map_sg returns the number of entries actually mapped, which
> might be smaller than the number passed in due to merging.
Yep, but the ubi_sql has a fixed number of scatterlist entries, UBI_MAX_SG_COUNT.
And I limit it also to that using: blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
Is there another reason why I should use the return value of blk_rq_map_sg()?
Please also note that the UBI block driver is read-only.
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-13 22:51 ` Richard Weinberger
@ 2015-01-13 22:54 ` Jens Axboe
-1 siblings, 0 replies; 32+ messages in thread
From: Jens Axboe @ 2015-01-13 22:54 UTC (permalink / raw)
To: Richard Weinberger, Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
tom.leiming
On 01/13/2015 03:51 PM, Richard Weinberger wrote:
> Am 13.01.2015 um 17:25 schrieb Christoph Hellwig:
>>> + struct ubi_sgl usgl;
>>
>> Btw, what's in struct ubi_sgl? Can't find that in my tree.
>
> "[PATCH 1/2] UBI: Add initial support for scatter gather" adds it.
>
> /**
> * struct ubi_sgl - UBI scatter gather list data structure.
> * @list_pos: current position in @sg[]
> * @page_pos: current position in @sg[@list_pos]
> * @sg: the scatter gather list itself
> *
> * ubi_sgl is a wrapper around a scatter list which keeps track of the
> * current position in the list and the current list item such that
> * it can be used across multiple ubi_leb_read_sg() calls.
> */
> struct ubi_sgl {
> int list_pos;
> int page_pos;
> struct scatterlist sg[UBI_MAX_SG_COUNT];
> };
>
>>> +static void ubiblock_do_work(struct work_struct *work)
>>> +{
>>> + int ret;
>>> + struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
>>> + struct request *req = blk_mq_rq_from_pdu(pdu);
>>> +
>>> + blk_mq_start_request(req);
>>> + blk_rq_map_sg(req->q, req, pdu->usgl.sg);
>>
>> blk_rq_map_sg returns the number of entries actually mapped, which
>> might be smaller than the number passed in due to merging.
>
> Yep, but the ubi_sql has a fixed number of scatterlist entries, UBI_MAX_SG_COUNT.
> And I limit it also to that using: blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
>
> Is there another reason why I should use the return value of blk_rq_map_sg()?
> Please also note that the UBI block driver is read-only.
It can return less than what you asked for, if segments are coalesced.
Read/write, doesn't matter. You should always use the returned value as
the indication of how many segments to access in pdu->usgl.sg for data
transfer.
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-13 22:54 ` Jens Axboe
0 siblings, 0 replies; 32+ messages in thread
From: Jens Axboe @ 2015-01-13 22:54 UTC (permalink / raw)
To: Richard Weinberger, Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, linux-mtd,
computersforpeace, dwmw2
On 01/13/2015 03:51 PM, Richard Weinberger wrote:
> Am 13.01.2015 um 17:25 schrieb Christoph Hellwig:
>>> + struct ubi_sgl usgl;
>>
>> Btw, what's in struct ubi_sgl? Can't find that in my tree.
>
> "[PATCH 1/2] UBI: Add initial support for scatter gather" adds it.
>
> /**
> * struct ubi_sgl - UBI scatter gather list data structure.
> * @list_pos: current position in @sg[]
> * @page_pos: current position in @sg[@list_pos]
> * @sg: the scatter gather list itself
> *
> * ubi_sgl is a wrapper around a scatter list which keeps track of the
> * current position in the list and the current list item such that
> * it can be used across multiple ubi_leb_read_sg() calls.
> */
> struct ubi_sgl {
> int list_pos;
> int page_pos;
> struct scatterlist sg[UBI_MAX_SG_COUNT];
> };
>
>>> +static void ubiblock_do_work(struct work_struct *work)
>>> +{
>>> + int ret;
>>> + struct ubiblock_pdu *pdu = container_of(work, struct ubiblock_pdu, work);
>>> + struct request *req = blk_mq_rq_from_pdu(pdu);
>>> +
>>> + blk_mq_start_request(req);
>>> + blk_rq_map_sg(req->q, req, pdu->usgl.sg);
>>
>> blk_rq_map_sg returns the number of entries actually mapped, which
>> might be smaller than the number passed in due to merging.
>
> Yep, but the ubi_sql has a fixed number of scatterlist entries, UBI_MAX_SG_COUNT.
> And I limit it also to that using: blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
>
> Is there another reason why I should use the return value of blk_rq_map_sg()?
> Please also note that the UBI block driver is read-only.
It can return less than what you asked for, if segments are coalesced.
Read/write, doesn't matter. You should always use the returned value as
the indication of how many segments to access in pdu->usgl.sg for data
transfer.
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-13 22:54 ` Jens Axboe
@ 2015-01-13 23:17 ` Richard Weinberger
-1 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-13 23:17 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
tom.leiming
Am 13.01.2015 um 23:54 schrieb Jens Axboe:
>>> blk_rq_map_sg returns the number of entries actually mapped, which
>>> might be smaller than the number passed in due to merging.
>>
>> Yep, but the ubi_sql has a fixed number of scatterlist entries, UBI_MAX_SG_COUNT.
>> And I limit it also to that using: blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
>>
>> Is there another reason why I should use the return value of blk_rq_map_sg()?
>> Please also note that the UBI block driver is read-only.
>
> It can return less than what you asked for, if segments are coalesced.
> Read/write, doesn't matter. You should always use the returned value as
> the indication of how many segments to access in pdu->usgl.sg for data
> transfer.
Sorry, I don't fully understand.
Currently the driver does:
to_read = blk_rq_bytes(req);
Then it fills pdu->usgl.sg up to to_read bytes
and calls blk_mq_end_request().
If I understand you correctly it can happen that blk_rq_bytes() returns
more bytes than blk_rq_map_sg() allocated, right?
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-13 23:17 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-13 23:17 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, linux-mtd,
computersforpeace, dwmw2
Am 13.01.2015 um 23:54 schrieb Jens Axboe:
>>> blk_rq_map_sg returns the number of entries actually mapped, which
>>> might be smaller than the number passed in due to merging.
>>
>> Yep, but the ubi_sql has a fixed number of scatterlist entries, UBI_MAX_SG_COUNT.
>> And I limit it also to that using: blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
>>
>> Is there another reason why I should use the return value of blk_rq_map_sg()?
>> Please also note that the UBI block driver is read-only.
>
> It can return less than what you asked for, if segments are coalesced.
> Read/write, doesn't matter. You should always use the returned value as
> the indication of how many segments to access in pdu->usgl.sg for data
> transfer.
Sorry, I don't fully understand.
Currently the driver does:
to_read = blk_rq_bytes(req);
Then it fills pdu->usgl.sg up to to_read bytes
and calls blk_mq_end_request().
If I understand you correctly it can happen that blk_rq_bytes() returns
more bytes than blk_rq_map_sg() allocated, right?
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-13 23:17 ` Richard Weinberger
@ 2015-01-13 23:30 ` Jens Axboe
-1 siblings, 0 replies; 32+ messages in thread
From: Jens Axboe @ 2015-01-13 23:30 UTC (permalink / raw)
To: Richard Weinberger, Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
tom.leiming
On 01/13/2015 04:17 PM, Richard Weinberger wrote:
> Am 13.01.2015 um 23:54 schrieb Jens Axboe:
>>>> blk_rq_map_sg returns the number of entries actually mapped, which
>>>> might be smaller than the number passed in due to merging.
>>>
>>> Yep, but the ubi_sql has a fixed number of scatterlist entries, UBI_MAX_SG_COUNT.
>>> And I limit it also to that using: blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
>>>
>>> Is there another reason why I should use the return value of blk_rq_map_sg()?
>>> Please also note that the UBI block driver is read-only.
>>
>> It can return less than what you asked for, if segments are coalesced.
>> Read/write, doesn't matter. You should always use the returned value as
>> the indication of how many segments to access in pdu->usgl.sg for data
>> transfer.
>
> Sorry, I don't fully understand.
>
> Currently the driver does:
> to_read = blk_rq_bytes(req);
> Then it fills pdu->usgl.sg up to to_read bytes
> and calls blk_mq_end_request().
>
> If I understand you correctly it can happen that blk_rq_bytes() returns
> more bytes than blk_rq_map_sg() allocated, right?
No, the number of bytes will be the same, no magic is involved :-)
But lets say the initial request has 4 bios, with each 2 pages, for a
total of 8 segments. Lets further assume that the pages in each bio are
contiguous, so that blk_rq_map_sg() will map this to 4 sg elements, each
2xpages long.
Now, this may already be handled just fine, and you don't need to
update/store the actual sg count. I just looked at the source, and I'm
assuming it'll do the right thing (ubi_read_sg() will bump the active sg
element, when that size has been consumed), but I don't have
ubi_read_sg() in my tree to verify.
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-13 23:30 ` Jens Axboe
0 siblings, 0 replies; 32+ messages in thread
From: Jens Axboe @ 2015-01-13 23:30 UTC (permalink / raw)
To: Richard Weinberger, Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, linux-mtd,
computersforpeace, dwmw2
On 01/13/2015 04:17 PM, Richard Weinberger wrote:
> Am 13.01.2015 um 23:54 schrieb Jens Axboe:
>>>> blk_rq_map_sg returns the number of entries actually mapped, which
>>>> might be smaller than the number passed in due to merging.
>>>
>>> Yep, but the ubi_sql has a fixed number of scatterlist entries, UBI_MAX_SG_COUNT.
>>> And I limit it also to that using: blk_queue_max_segments(dev->rq, UBI_MAX_SG_COUNT);
>>>
>>> Is there another reason why I should use the return value of blk_rq_map_sg()?
>>> Please also note that the UBI block driver is read-only.
>>
>> It can return less than what you asked for, if segments are coalesced.
>> Read/write, doesn't matter. You should always use the returned value as
>> the indication of how many segments to access in pdu->usgl.sg for data
>> transfer.
>
> Sorry, I don't fully understand.
>
> Currently the driver does:
> to_read = blk_rq_bytes(req);
> Then it fills pdu->usgl.sg up to to_read bytes
> and calls blk_mq_end_request().
>
> If I understand you correctly it can happen that blk_rq_bytes() returns
> more bytes than blk_rq_map_sg() allocated, right?
No, the number of bytes will be the same, no magic is involved :-)
But lets say the initial request has 4 bios, with each 2 pages, for a
total of 8 segments. Lets further assume that the pages in each bio are
contiguous, so that blk_rq_map_sg() will map this to 4 sg elements, each
2xpages long.
Now, this may already be handled just fine, and you don't need to
update/store the actual sg count. I just looked at the source, and I'm
assuming it'll do the right thing (ubi_read_sg() will bump the active sg
element, when that size has been consumed), but I don't have
ubi_read_sg() in my tree to verify.
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-13 23:30 ` Jens Axboe
@ 2015-01-13 23:36 ` Richard Weinberger
-1 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-13 23:36 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
tom.leiming
Am 14.01.2015 um 00:30 schrieb Jens Axboe:
>> If I understand you correctly it can happen that blk_rq_bytes() returns
>> more bytes than blk_rq_map_sg() allocated, right?
>
> No, the number of bytes will be the same, no magic is involved :-)
Good to know. :)
> But lets say the initial request has 4 bios, with each 2 pages, for a
> total of 8 segments. Lets further assume that the pages in each bio are
> contiguous, so that blk_rq_map_sg() will map this to 4 sg elements, each
> 2xpages long.
>
> Now, this may already be handled just fine, and you don't need to
> update/store the actual sg count. I just looked at the source, and I'm
> assuming it'll do the right thing (ubi_read_sg() will bump the active sg
> element, when that size has been consumed), but I don't have
> ubi_read_sg() in my tree to verify.
Currently the sg count is hard coded to UBI_MAX_SG_COUNT.
I'm sorry, I forgot to CC you and hch to this patch:
https://lkml.org/lkml/2015/1/10/204
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-13 23:36 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-13 23:36 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, linux-mtd,
computersforpeace, dwmw2
Am 14.01.2015 um 00:30 schrieb Jens Axboe:
>> If I understand you correctly it can happen that blk_rq_bytes() returns
>> more bytes than blk_rq_map_sg() allocated, right?
>
> No, the number of bytes will be the same, no magic is involved :-)
Good to know. :)
> But lets say the initial request has 4 bios, with each 2 pages, for a
> total of 8 segments. Lets further assume that the pages in each bio are
> contiguous, so that blk_rq_map_sg() will map this to 4 sg elements, each
> 2xpages long.
>
> Now, this may already be handled just fine, and you don't need to
> update/store the actual sg count. I just looked at the source, and I'm
> assuming it'll do the right thing (ubi_read_sg() will bump the active sg
> element, when that size has been consumed), but I don't have
> ubi_read_sg() in my tree to verify.
Currently the sg count is hard coded to UBI_MAX_SG_COUNT.
I'm sorry, I forgot to CC you and hch to this patch:
https://lkml.org/lkml/2015/1/10/204
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-13 23:36 ` Richard Weinberger
@ 2015-01-14 0:23 ` Jens Axboe
-1 siblings, 0 replies; 32+ messages in thread
From: Jens Axboe @ 2015-01-14 0:23 UTC (permalink / raw)
To: Richard Weinberger, Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
tom.leiming
On 01/13/2015 04:36 PM, Richard Weinberger wrote:
>
>
> Am 14.01.2015 um 00:30 schrieb Jens Axboe:
>>> If I understand you correctly it can happen that blk_rq_bytes() returns
>>> more bytes than blk_rq_map_sg() allocated, right?
>>
>> No, the number of bytes will be the same, no magic is involved :-)
>
> Good to know. :)
>
>> But lets say the initial request has 4 bios, with each 2 pages, for a
>> total of 8 segments. Lets further assume that the pages in each bio are
>> contiguous, so that blk_rq_map_sg() will map this to 4 sg elements, each
>> 2xpages long.
>>
>> Now, this may already be handled just fine, and you don't need to
>> update/store the actual sg count. I just looked at the source, and I'm
>> assuming it'll do the right thing (ubi_read_sg() will bump the active sg
>> element, when that size has been consumed), but I don't have
>> ubi_read_sg() in my tree to verify.
>
> Currently the sg count is hard coded to UBI_MAX_SG_COUNT.
The max count doesn't matter, that just provides you a guarantee that
you'll never receive a request that maps to more than that. The point
I'm trying to make is that if you receive 8 segments and it maps to 4,
then you better not look at segments 5..8 after it being mapped.
Whatever the max is, doesn't matter in this conversation.
> I'm sorry, I forgot to CC you and hch to this patch:
Which is as I suspected, you'll do each segment to the length specified,
hence you don't need to track the returned count from blk_rq_map_sg().
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-14 0:23 ` Jens Axboe
0 siblings, 0 replies; 32+ messages in thread
From: Jens Axboe @ 2015-01-14 0:23 UTC (permalink / raw)
To: Richard Weinberger, Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, linux-mtd,
computersforpeace, dwmw2
On 01/13/2015 04:36 PM, Richard Weinberger wrote:
>
>
> Am 14.01.2015 um 00:30 schrieb Jens Axboe:
>>> If I understand you correctly it can happen that blk_rq_bytes() returns
>>> more bytes than blk_rq_map_sg() allocated, right?
>>
>> No, the number of bytes will be the same, no magic is involved :-)
>
> Good to know. :)
>
>> But lets say the initial request has 4 bios, with each 2 pages, for a
>> total of 8 segments. Lets further assume that the pages in each bio are
>> contiguous, so that blk_rq_map_sg() will map this to 4 sg elements, each
>> 2xpages long.
>>
>> Now, this may already be handled just fine, and you don't need to
>> update/store the actual sg count. I just looked at the source, and I'm
>> assuming it'll do the right thing (ubi_read_sg() will bump the active sg
>> element, when that size has been consumed), but I don't have
>> ubi_read_sg() in my tree to verify.
>
> Currently the sg count is hard coded to UBI_MAX_SG_COUNT.
The max count doesn't matter, that just provides you a guarantee that
you'll never receive a request that maps to more than that. The point
I'm trying to make is that if you receive 8 segments and it maps to 4,
then you better not look at segments 5..8 after it being mapped.
Whatever the max is, doesn't matter in this conversation.
> I'm sorry, I forgot to CC you and hch to this patch:
Which is as I suspected, you'll do each segment to the length specified,
hence you don't need to track the returned count from blk_rq_map_sg().
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-14 0:23 ` Jens Axboe
@ 2015-01-14 8:39 ` Richard Weinberger
-1 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-14 8:39 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
tom.leiming
Am 14.01.2015 um 01:23 schrieb Jens Axboe:
> On 01/13/2015 04:36 PM, Richard Weinberger wrote:
>>
>>
>> Am 14.01.2015 um 00:30 schrieb Jens Axboe:
>>>> If I understand you correctly it can happen that blk_rq_bytes() returns
>>>> more bytes than blk_rq_map_sg() allocated, right?
>>>
>>> No, the number of bytes will be the same, no magic is involved :-)
>>
>> Good to know. :)
>>
>>> But lets say the initial request has 4 bios, with each 2 pages, for a
>>> total of 8 segments. Lets further assume that the pages in each bio are
>>> contiguous, so that blk_rq_map_sg() will map this to 4 sg elements, each
>>> 2xpages long.
>>>
>>> Now, this may already be handled just fine, and you don't need to
>>> update/store the actual sg count. I just looked at the source, and I'm
>>> assuming it'll do the right thing (ubi_read_sg() will bump the active sg
>>> element, when that size has been consumed), but I don't have
>>> ubi_read_sg() in my tree to verify.
>>
>> Currently the sg count is hard coded to UBI_MAX_SG_COUNT.
>
> The max count doesn't matter, that just provides you a guarantee that
> you'll never receive a request that maps to more than that. The point
> I'm trying to make is that if you receive 8 segments and it maps to 4,
> then you better not look at segments 5..8 after it being mapped.
> Whatever the max is, doesn't matter in this conversation.
>
>> I'm sorry, I forgot to CC you and hch to this patch:
>
> Which is as I suspected, you'll do each segment to the length specified,
> hence you don't need to track the returned count from blk_rq_map_sg().
Thanks a lot for the kind explanation, Jens!
I'll add a comment the usage of blk_rq_map_sg() to avoid further confusion.
Am I allowed to add your Reviewed-by too?
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-14 8:39 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-14 8:39 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, linux-mtd,
computersforpeace, dwmw2
Am 14.01.2015 um 01:23 schrieb Jens Axboe:
> On 01/13/2015 04:36 PM, Richard Weinberger wrote:
>>
>>
>> Am 14.01.2015 um 00:30 schrieb Jens Axboe:
>>>> If I understand you correctly it can happen that blk_rq_bytes() returns
>>>> more bytes than blk_rq_map_sg() allocated, right?
>>>
>>> No, the number of bytes will be the same, no magic is involved :-)
>>
>> Good to know. :)
>>
>>> But lets say the initial request has 4 bios, with each 2 pages, for a
>>> total of 8 segments. Lets further assume that the pages in each bio are
>>> contiguous, so that blk_rq_map_sg() will map this to 4 sg elements, each
>>> 2xpages long.
>>>
>>> Now, this may already be handled just fine, and you don't need to
>>> update/store the actual sg count. I just looked at the source, and I'm
>>> assuming it'll do the right thing (ubi_read_sg() will bump the active sg
>>> element, when that size has been consumed), but I don't have
>>> ubi_read_sg() in my tree to verify.
>>
>> Currently the sg count is hard coded to UBI_MAX_SG_COUNT.
>
> The max count doesn't matter, that just provides you a guarantee that
> you'll never receive a request that maps to more than that. The point
> I'm trying to make is that if you receive 8 segments and it maps to 4,
> then you better not look at segments 5..8 after it being mapped.
> Whatever the max is, doesn't matter in this conversation.
>
>> I'm sorry, I forgot to CC you and hch to this patch:
>
> Which is as I suspected, you'll do each segment to the length specified,
> hence you don't need to track the returned count from blk_rq_map_sg().
Thanks a lot for the kind explanation, Jens!
I'll add a comment the usage of blk_rq_map_sg() to avoid further confusion.
Am I allowed to add your Reviewed-by too?
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2 v2] UBI: Add initial support for scatter gather
2015-01-10 21:52 ` Richard Weinberger
@ 2015-01-26 23:53 ` Richard Weinberger
-1 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-26 23:53 UTC (permalink / raw)
To: dedekind1
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel,
Ezequiel Garcia, Guido Martínez
Am 10.01.2015 um 22:52 schrieb Richard Weinberger:
> Adds a new set of functions to deal with scatter gather.
> ubi_eba_read_leb_sg() will read from a LEB into a scatter gather list.
> The new data structure struct ubi_sgl will be used within UBI to
> hold the scatter gather list itself and metadata to have a cursor
> within the list.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Can I please get a Reviewed-by for this patch?
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2 v2] UBI: Add initial support for scatter gather
@ 2015-01-26 23:53 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-26 23:53 UTC (permalink / raw)
To: dedekind1
Cc: linux-kernel, linux-mtd, Ezequiel Garcia, computersforpeace,
dwmw2, Guido Martínez
Am 10.01.2015 um 22:52 schrieb Richard Weinberger:
> Adds a new set of functions to deal with scatter gather.
> ubi_eba_read_leb_sg() will read from a LEB into a scatter gather list.
> The new data structure struct ubi_sgl will be used within UBI to
> hold the scatter gather list itself and metadata to have a cursor
> within the list.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Can I please get a Reviewed-by for this patch?
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-14 8:39 ` Richard Weinberger
@ 2015-01-26 23:55 ` Richard Weinberger
-1 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-26 23:55 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
tom.leiming
Am 14.01.2015 um 09:39 schrieb Richard Weinberger:
> Am 14.01.2015 um 01:23 schrieb Jens Axboe:
>> Which is as I suspected, you'll do each segment to the length specified,
>> hence you don't need to track the returned count from blk_rq_map_sg().
>
> Thanks a lot for the kind explanation, Jens!
> I'll add a comment the usage of blk_rq_map_sg() to avoid further confusion.
> Am I allowed to add your Reviewed-by too?
Ping?
I'd like to add a Reviewed-by to this patch.
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-26 23:55 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-26 23:55 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, linux-mtd,
computersforpeace, dwmw2
Am 14.01.2015 um 09:39 schrieb Richard Weinberger:
> Am 14.01.2015 um 01:23 schrieb Jens Axboe:
>> Which is as I suspected, you'll do each segment to the length specified,
>> hence you don't need to track the returned count from blk_rq_map_sg().
>
> Thanks a lot for the kind explanation, Jens!
> I'll add a comment the usage of blk_rq_map_sg() to avoid further confusion.
> Am I allowed to add your Reviewed-by too?
Ping?
I'd like to add a Reviewed-by to this patch.
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-26 23:55 ` Richard Weinberger
@ 2015-01-27 4:03 ` Jens Axboe
-1 siblings, 0 replies; 32+ messages in thread
From: Jens Axboe @ 2015-01-27 4:03 UTC (permalink / raw)
To: Richard Weinberger, Christoph Hellwig
Cc: dedekind1, dwmw2, computersforpeace, linux-mtd, linux-kernel,
tom.leiming
On 01/26/2015 04:55 PM, Richard Weinberger wrote:
> Am 14.01.2015 um 09:39 schrieb Richard Weinberger:
>> Am 14.01.2015 um 01:23 schrieb Jens Axboe:
>>> Which is as I suspected, you'll do each segment to the length specified,
>>> hence you don't need to track the returned count from blk_rq_map_sg().
>>
>> Thanks a lot for the kind explanation, Jens!
>> I'll add a comment the usage of blk_rq_map_sg() to avoid further confusion.
>> Am I allowed to add your Reviewed-by too?
>
> Ping?
> I'd like to add a Reviewed-by to this patch.
Yup sorry, looks OK to go in from my point of view. You can add my
reviewed-by.
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-27 4:03 ` Jens Axboe
0 siblings, 0 replies; 32+ messages in thread
From: Jens Axboe @ 2015-01-27 4:03 UTC (permalink / raw)
To: Richard Weinberger, Christoph Hellwig
Cc: dedekind1, tom.leiming, linux-kernel, linux-mtd,
computersforpeace, dwmw2
On 01/26/2015 04:55 PM, Richard Weinberger wrote:
> Am 14.01.2015 um 09:39 schrieb Richard Weinberger:
>> Am 14.01.2015 um 01:23 schrieb Jens Axboe:
>>> Which is as I suspected, you'll do each segment to the length specified,
>>> hence you don't need to track the returned count from blk_rq_map_sg().
>>
>> Thanks a lot for the kind explanation, Jens!
>> I'll add a comment the usage of blk_rq_map_sg() to avoid further confusion.
>> Am I allowed to add your Reviewed-by too?
>
> Ping?
> I'd like to add a Reviewed-by to this patch.
Yup sorry, looks OK to go in from my point of view. You can add my
reviewed-by.
--
Jens Axboe
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2 v2] UBI: Add initial support for scatter gather
2015-01-10 21:52 ` Richard Weinberger
@ 2015-01-27 23:36 ` Ezequiel Garcia
-1 siblings, 0 replies; 32+ messages in thread
From: Ezequiel Garcia @ 2015-01-27 23:36 UTC (permalink / raw)
To: Richard Weinberger, dedekind1
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel
On 01/10/2015 06:52 PM, Richard Weinberger wrote:
> Adds a new set of functions to deal with scatter gather.
> ubi_eba_read_leb_sg() will read from a LEB into a scatter gather list.
> The new data structure struct ubi_sgl will be used within UBI to
> hold the scatter gather list itself and metadata to have a cursor
> within the list.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Three nitpicks below, and my
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> drivers/mtd/ubi/eba.c | 56 +++++++++++++++++++++++++++++
> drivers/mtd/ubi/kapi.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
> drivers/mtd/ubi/ubi.h | 3 ++
> include/linux/mtd/ubi.h | 46 ++++++++++++++++++++++++
> 4 files changed, 185 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> index b698534..0aaf707 100644
> --- a/drivers/mtd/ubi/eba.c
> +++ b/drivers/mtd/ubi/eba.c
> @@ -481,6 +481,62 @@ out_unlock:
> }
>
> /**
> + * ubi_eba_read_leb_sg - read data into a scatter gather list.
> + * @ubi: UBI device description object
> + * @vol: volume description object
> + * @lnum: logical eraseblock number
> + * @sgl: UBI scatter gather list to store the read data
> + * @offset: offset from where to read
> + * @len: how many bytes to read
> + * @check: data CRC check flag
> + *
> + * This function works exactly like ubi_eba_read_leb(). But instead of
> + * storing the read data into a buffer it writes to an UBI scatter gather
> + * list.
> + */
> +int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
> + struct ubi_sgl *sgl, int lnum, int offset, int len,
> + int check)
> +{
> + int to_read;
> + int ret;
> + struct scatterlist *sg;
> +
> + for (;;) {
> + ubi_assert(sgl->list_pos < UBI_MAX_SG_COUNT);
> + sg = &sgl->sg[sgl->list_pos];
> + if (len < sg->length - sgl->page_pos)
> + to_read = len;
> + else
> + to_read = sg->length - sgl->page_pos;
> +
> + ret = ubi_eba_read_leb(ubi, vol, lnum,
> + sg_virt(sg) + sgl->page_pos, offset,
> + to_read, check);
> + if (ret < 0)
> + goto out;
Matter of taste, but isn't it better to just return here?
> +
> + offset += to_read;
> + len -= to_read;
> + if (!len) {
> + sgl->page_pos += to_read;
> + if (sgl->page_pos == sg->length) {
> + sgl->list_pos++;
> + sgl->page_pos = 0;
> + }
> +
> + break;
> + }
> +
> + sgl->list_pos++;
> + sgl->page_pos = 0;
> + }
> +
> +out:
> + return ret;
> +}
> +
> +/**
> * recover_peb - recover from write failure.
> * @ubi: UBI device description object
> * @pnum: the physical eraseblock to recover
> diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
> index f3bab66..d0055a2 100644
> --- a/drivers/mtd/ubi/kapi.c
> +++ b/drivers/mtd/ubi/kapi.c
> @@ -355,6 +355,43 @@ void ubi_close_volume(struct ubi_volume_desc *desc)
> EXPORT_SYMBOL_GPL(ubi_close_volume);
>
> /**
> + * leb_read_sanity_check - does sanity checks on read requests.
> + * @desc: volume descriptor
> + * @lnum: logical eraseblock number to read from
> + * @offset: offset within the logical eraseblock to read from
> + * @len: how many bytes to read
> + *
> + * This function is used by ubi_leb_read() and ubi_leb_read_sg()
> + * to perfrom sanity checks.
s/perfrom/perform
> + */
> +static int leb_read_sanity_check(struct ubi_volume_desc *desc, int lnum,
> + int offset, int len)
> +{
> + struct ubi_volume *vol = desc->vol;
> + struct ubi_device *ubi = vol->ubi;
> + int vol_id = vol->vol_id;
> +
> + if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
> + lnum >= vol->used_ebs || offset < 0 || len < 0 ||
> + offset + len > vol->usable_leb_size)
> + return -EINVAL;
> +
> + if (vol->vol_type == UBI_STATIC_VOLUME) {
> + if (vol->used_ebs == 0)
> + /* Empty static UBI volume */
> + return 0;
> + if (lnum == vol->used_ebs - 1 &&
> + offset + len > vol->last_eb_bytes)
> + return -EINVAL;
> + }
> +
> + if (vol->upd_marker)
> + return -EBADF;
> +
> + return 0;
> +}
> +
> +/**
> * ubi_leb_read - read data.
> * @desc: volume descriptor
> * @lnum: logical eraseblock number to read from
> @@ -390,22 +427,10 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
>
> dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
>
> - if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
> - lnum >= vol->used_ebs || offset < 0 || len < 0 ||
> - offset + len > vol->usable_leb_size)
> - return -EINVAL;
> -
> - if (vol->vol_type == UBI_STATIC_VOLUME) {
> - if (vol->used_ebs == 0)
> - /* Empty static UBI volume */
> - return 0;
> - if (lnum == vol->used_ebs - 1 &&
> - offset + len > vol->last_eb_bytes)
> - return -EINVAL;
> - }
> + err = leb_read_sanity_check(desc, lnum, offset, len);
> + if (err < 0)
> + return err;
>
> - if (vol->upd_marker)
> - return -EBADF;
> if (len == 0)
> return 0;
>
> @@ -419,6 +444,46 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
> }
> EXPORT_SYMBOL_GPL(ubi_leb_read);
>
> +
> +/**
> + * ubi_leb_read_sg - read data into a scatter gather list.
> + * @desc: volume descriptor
> + * @lnum: logical eraseblock number to read from
> + * @buf: buffer where to store the read data
> + * @offset: offset within the logical eraseblock to read from
> + * @len: how many bytes to read
> + * @check: whether UBI has to check the read data's CRC or not.
> + *
> + * This function works exactly like ubi_leb_read_sg(). But instead of
> + * storing the read data into a buffer it writes to an UBI scatter gather
> + * list.
> + */
> +int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
> + int offset, int len, int check)
> +{
> + struct ubi_volume *vol = desc->vol;
> + struct ubi_device *ubi = vol->ubi;
> + int err, vol_id = vol->vol_id;
> +
> + dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
> +
> + err = leb_read_sanity_check(desc, lnum, offset, len);
> + if (err < 0)
> + return err;
> +
> + if (len == 0)
> + return 0;
> +
> + err = ubi_eba_read_leb_sg(ubi, vol, sgl, lnum, offset, len, check);
> + if (err && mtd_is_eccerr(err) && vol->vol_type == UBI_STATIC_VOLUME) {
> + ubi_warn(ubi, "mark volume %d as corrupted", vol_id);
> + vol->corrupted = 1;
> + }
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(ubi_leb_read_sg);
> +
> /**
> * ubi_leb_write - write data.
> * @desc: volume descriptor
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index ee7ac0b..a5283cf 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -791,6 +791,9 @@ int ubi_eba_unmap_leb(struct ubi_device *ubi, struct ubi_volume *vol,
> int lnum);
> int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
> void *buf, int offset, int len, int check);
> +int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
> + struct ubi_sgl *sgl, int lnum, int offset, int len,
> + int check);
> int ubi_eba_write_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
> const void *buf, int offset, int len);
> int ubi_eba_write_leb_st(struct ubi_device *ubi, struct ubi_volume *vol,
> diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
> index c3918a0..3d5916d 100644
> --- a/include/linux/mtd/ubi.h
> +++ b/include/linux/mtd/ubi.h
> @@ -23,11 +23,16 @@
>
> #include <linux/ioctl.h>
> #include <linux/types.h>
> +#include <linux/scatterlist.h>
> #include <mtd/ubi-user.h>
>
> /* All voumes/LEBs */
> #define UBI_ALL -1
>
> +/* Maximum number of scatter gather list entries,
> + * we use only 64 to have a lower memory foot print. */
Multilines comments style is:
/*
*
*/
> +#define UBI_MAX_SG_COUNT 64
> +
> /*
> * enum ubi_open_mode - UBI volume open mode constants.
> *
> @@ -116,6 +121,35 @@ struct ubi_volume_info {
> };
>
> /**
> + * struct ubi_sgl - UBI scatter gather list data structure.
> + * @list_pos: current position in @sg[]
> + * @page_pos: current position in @sg[@list_pos]
> + * @sg: the scatter gather list itself
> + *
> + * ubi_sgl is a wrapper around a scatter list which keeps track of the
> + * current position in the list and the current list item such that
> + * it can be used across multiple ubi_leb_read_sg() calls.
> + */
> +struct ubi_sgl {
> + int list_pos;
> + int page_pos;
> + struct scatterlist sg[UBI_MAX_SG_COUNT];
> +};
> +
> +/**
> + * ubi_sgl_init - initialize an UBI scatter gather list data structure.
> + * @usgl: the UBI scatter gather struct itself
> + *
> + * Please note that you still have to use sg_init_table() or any adequate
> + * function to initialize the unterlaying struct scatterlist.
> + */
> +static inline void ubi_sgl_init(struct ubi_sgl *usgl)
> +{
> + usgl->list_pos = 0;
> + usgl->page_pos = 0;
> +}
> +
> +/**
> * struct ubi_device_info - UBI device description data structure.
> * @ubi_num: ubi device number
> * @leb_size: logical eraseblock size on this UBI device
> @@ -210,6 +244,8 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb);
> void ubi_close_volume(struct ubi_volume_desc *desc);
> int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
> int len, int check);
> +int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
> + int offset, int len, int check);
> int ubi_leb_write(struct ubi_volume_desc *desc, int lnum, const void *buf,
> int offset, int len);
> int ubi_leb_change(struct ubi_volume_desc *desc, int lnum, const void *buf,
> @@ -230,4 +266,14 @@ static inline int ubi_read(struct ubi_volume_desc *desc, int lnum, char *buf,
> {
> return ubi_leb_read(desc, lnum, buf, offset, len, 0);
> }
> +
> +/*
> + * This function is the same as the 'ubi_leb_read_sg()' function, but it does
> + * not provide the checking capability.
> + */
> +static inline int ubi_read_sg(struct ubi_volume_desc *desc, int lnum,
> + struct ubi_sgl *sgl, int offset, int len)
> +{
> + return ubi_leb_read_sg(desc, lnum, sgl, offset, len, 0);
> +}
> #endif /* !__LINUX_UBI_H__ */
>
--
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2 v2] UBI: Add initial support for scatter gather
@ 2015-01-27 23:36 ` Ezequiel Garcia
0 siblings, 0 replies; 32+ messages in thread
From: Ezequiel Garcia @ 2015-01-27 23:36 UTC (permalink / raw)
To: Richard Weinberger, dedekind1
Cc: linux-mtd, computersforpeace, dwmw2, linux-kernel
On 01/10/2015 06:52 PM, Richard Weinberger wrote:
> Adds a new set of functions to deal with scatter gather.
> ubi_eba_read_leb_sg() will read from a LEB into a scatter gather list.
> The new data structure struct ubi_sgl will be used within UBI to
> hold the scatter gather list itself and metadata to have a cursor
> within the list.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Three nitpicks below, and my
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> drivers/mtd/ubi/eba.c | 56 +++++++++++++++++++++++++++++
> drivers/mtd/ubi/kapi.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
> drivers/mtd/ubi/ubi.h | 3 ++
> include/linux/mtd/ubi.h | 46 ++++++++++++++++++++++++
> 4 files changed, 185 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> index b698534..0aaf707 100644
> --- a/drivers/mtd/ubi/eba.c
> +++ b/drivers/mtd/ubi/eba.c
> @@ -481,6 +481,62 @@ out_unlock:
> }
>
> /**
> + * ubi_eba_read_leb_sg - read data into a scatter gather list.
> + * @ubi: UBI device description object
> + * @vol: volume description object
> + * @lnum: logical eraseblock number
> + * @sgl: UBI scatter gather list to store the read data
> + * @offset: offset from where to read
> + * @len: how many bytes to read
> + * @check: data CRC check flag
> + *
> + * This function works exactly like ubi_eba_read_leb(). But instead of
> + * storing the read data into a buffer it writes to an UBI scatter gather
> + * list.
> + */
> +int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
> + struct ubi_sgl *sgl, int lnum, int offset, int len,
> + int check)
> +{
> + int to_read;
> + int ret;
> + struct scatterlist *sg;
> +
> + for (;;) {
> + ubi_assert(sgl->list_pos < UBI_MAX_SG_COUNT);
> + sg = &sgl->sg[sgl->list_pos];
> + if (len < sg->length - sgl->page_pos)
> + to_read = len;
> + else
> + to_read = sg->length - sgl->page_pos;
> +
> + ret = ubi_eba_read_leb(ubi, vol, lnum,
> + sg_virt(sg) + sgl->page_pos, offset,
> + to_read, check);
> + if (ret < 0)
> + goto out;
Matter of taste, but isn't it better to just return here?
> +
> + offset += to_read;
> + len -= to_read;
> + if (!len) {
> + sgl->page_pos += to_read;
> + if (sgl->page_pos == sg->length) {
> + sgl->list_pos++;
> + sgl->page_pos = 0;
> + }
> +
> + break;
> + }
> +
> + sgl->list_pos++;
> + sgl->page_pos = 0;
> + }
> +
> +out:
> + return ret;
> +}
> +
> +/**
> * recover_peb - recover from write failure.
> * @ubi: UBI device description object
> * @pnum: the physical eraseblock to recover
> diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
> index f3bab66..d0055a2 100644
> --- a/drivers/mtd/ubi/kapi.c
> +++ b/drivers/mtd/ubi/kapi.c
> @@ -355,6 +355,43 @@ void ubi_close_volume(struct ubi_volume_desc *desc)
> EXPORT_SYMBOL_GPL(ubi_close_volume);
>
> /**
> + * leb_read_sanity_check - does sanity checks on read requests.
> + * @desc: volume descriptor
> + * @lnum: logical eraseblock number to read from
> + * @offset: offset within the logical eraseblock to read from
> + * @len: how many bytes to read
> + *
> + * This function is used by ubi_leb_read() and ubi_leb_read_sg()
> + * to perfrom sanity checks.
s/perfrom/perform
> + */
> +static int leb_read_sanity_check(struct ubi_volume_desc *desc, int lnum,
> + int offset, int len)
> +{
> + struct ubi_volume *vol = desc->vol;
> + struct ubi_device *ubi = vol->ubi;
> + int vol_id = vol->vol_id;
> +
> + if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
> + lnum >= vol->used_ebs || offset < 0 || len < 0 ||
> + offset + len > vol->usable_leb_size)
> + return -EINVAL;
> +
> + if (vol->vol_type == UBI_STATIC_VOLUME) {
> + if (vol->used_ebs == 0)
> + /* Empty static UBI volume */
> + return 0;
> + if (lnum == vol->used_ebs - 1 &&
> + offset + len > vol->last_eb_bytes)
> + return -EINVAL;
> + }
> +
> + if (vol->upd_marker)
> + return -EBADF;
> +
> + return 0;
> +}
> +
> +/**
> * ubi_leb_read - read data.
> * @desc: volume descriptor
> * @lnum: logical eraseblock number to read from
> @@ -390,22 +427,10 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
>
> dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
>
> - if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
> - lnum >= vol->used_ebs || offset < 0 || len < 0 ||
> - offset + len > vol->usable_leb_size)
> - return -EINVAL;
> -
> - if (vol->vol_type == UBI_STATIC_VOLUME) {
> - if (vol->used_ebs == 0)
> - /* Empty static UBI volume */
> - return 0;
> - if (lnum == vol->used_ebs - 1 &&
> - offset + len > vol->last_eb_bytes)
> - return -EINVAL;
> - }
> + err = leb_read_sanity_check(desc, lnum, offset, len);
> + if (err < 0)
> + return err;
>
> - if (vol->upd_marker)
> - return -EBADF;
> if (len == 0)
> return 0;
>
> @@ -419,6 +444,46 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
> }
> EXPORT_SYMBOL_GPL(ubi_leb_read);
>
> +
> +/**
> + * ubi_leb_read_sg - read data into a scatter gather list.
> + * @desc: volume descriptor
> + * @lnum: logical eraseblock number to read from
> + * @buf: buffer where to store the read data
> + * @offset: offset within the logical eraseblock to read from
> + * @len: how many bytes to read
> + * @check: whether UBI has to check the read data's CRC or not.
> + *
> + * This function works exactly like ubi_leb_read_sg(). But instead of
> + * storing the read data into a buffer it writes to an UBI scatter gather
> + * list.
> + */
> +int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
> + int offset, int len, int check)
> +{
> + struct ubi_volume *vol = desc->vol;
> + struct ubi_device *ubi = vol->ubi;
> + int err, vol_id = vol->vol_id;
> +
> + dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
> +
> + err = leb_read_sanity_check(desc, lnum, offset, len);
> + if (err < 0)
> + return err;
> +
> + if (len == 0)
> + return 0;
> +
> + err = ubi_eba_read_leb_sg(ubi, vol, sgl, lnum, offset, len, check);
> + if (err && mtd_is_eccerr(err) && vol->vol_type == UBI_STATIC_VOLUME) {
> + ubi_warn(ubi, "mark volume %d as corrupted", vol_id);
> + vol->corrupted = 1;
> + }
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(ubi_leb_read_sg);
> +
> /**
> * ubi_leb_write - write data.
> * @desc: volume descriptor
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index ee7ac0b..a5283cf 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -791,6 +791,9 @@ int ubi_eba_unmap_leb(struct ubi_device *ubi, struct ubi_volume *vol,
> int lnum);
> int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
> void *buf, int offset, int len, int check);
> +int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
> + struct ubi_sgl *sgl, int lnum, int offset, int len,
> + int check);
> int ubi_eba_write_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
> const void *buf, int offset, int len);
> int ubi_eba_write_leb_st(struct ubi_device *ubi, struct ubi_volume *vol,
> diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
> index c3918a0..3d5916d 100644
> --- a/include/linux/mtd/ubi.h
> +++ b/include/linux/mtd/ubi.h
> @@ -23,11 +23,16 @@
>
> #include <linux/ioctl.h>
> #include <linux/types.h>
> +#include <linux/scatterlist.h>
> #include <mtd/ubi-user.h>
>
> /* All voumes/LEBs */
> #define UBI_ALL -1
>
> +/* Maximum number of scatter gather list entries,
> + * we use only 64 to have a lower memory foot print. */
Multilines comments style is:
/*
*
*/
> +#define UBI_MAX_SG_COUNT 64
> +
> /*
> * enum ubi_open_mode - UBI volume open mode constants.
> *
> @@ -116,6 +121,35 @@ struct ubi_volume_info {
> };
>
> /**
> + * struct ubi_sgl - UBI scatter gather list data structure.
> + * @list_pos: current position in @sg[]
> + * @page_pos: current position in @sg[@list_pos]
> + * @sg: the scatter gather list itself
> + *
> + * ubi_sgl is a wrapper around a scatter list which keeps track of the
> + * current position in the list and the current list item such that
> + * it can be used across multiple ubi_leb_read_sg() calls.
> + */
> +struct ubi_sgl {
> + int list_pos;
> + int page_pos;
> + struct scatterlist sg[UBI_MAX_SG_COUNT];
> +};
> +
> +/**
> + * ubi_sgl_init - initialize an UBI scatter gather list data structure.
> + * @usgl: the UBI scatter gather struct itself
> + *
> + * Please note that you still have to use sg_init_table() or any adequate
> + * function to initialize the unterlaying struct scatterlist.
> + */
> +static inline void ubi_sgl_init(struct ubi_sgl *usgl)
> +{
> + usgl->list_pos = 0;
> + usgl->page_pos = 0;
> +}
> +
> +/**
> * struct ubi_device_info - UBI device description data structure.
> * @ubi_num: ubi device number
> * @leb_size: logical eraseblock size on this UBI device
> @@ -210,6 +244,8 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb);
> void ubi_close_volume(struct ubi_volume_desc *desc);
> int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
> int len, int check);
> +int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
> + int offset, int len, int check);
> int ubi_leb_write(struct ubi_volume_desc *desc, int lnum, const void *buf,
> int offset, int len);
> int ubi_leb_change(struct ubi_volume_desc *desc, int lnum, const void *buf,
> @@ -230,4 +266,14 @@ static inline int ubi_read(struct ubi_volume_desc *desc, int lnum, char *buf,
> {
> return ubi_leb_read(desc, lnum, buf, offset, len, 0);
> }
> +
> +/*
> + * This function is the same as the 'ubi_leb_read_sg()' function, but it does
> + * not provide the checking capability.
> + */
> +static inline int ubi_read_sg(struct ubi_volume_desc *desc, int lnum,
> + struct ubi_sgl *sgl, int offset, int len)
> +{
> + return ubi_leb_read_sg(desc, lnum, sgl, offset, len, 0);
> +}
> #endif /* !__LINUX_UBI_H__ */
>
--
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
2015-01-10 21:52 ` Richard Weinberger
@ 2015-01-27 23:37 ` Ezequiel Garcia
-1 siblings, 0 replies; 32+ messages in thread
From: Ezequiel Garcia @ 2015-01-27 23:37 UTC (permalink / raw)
To: Richard Weinberger, dedekind1
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, hch, axboe,
tom.leiming
On 01/10/2015 06:52 PM, Richard Weinberger wrote:
> Convert the driver to blk-mq.
> Beside of moving to the modern block interface this change boosts
> also the performance of the driver.
>
> nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
> nand: Micron NAND 256MiB 3,3V 8-bit
> nand: 256MiB, SLC, page size: 2048, OOB size: 64
>
> root@debian-armhf:~# dd if=/dev/ubiblock0_0 of=/dev/zero bs=1M
> 243+1 records in
> 243+1 records out
> 255080448 bytes (255 MB) copied, 4.39295 s, 58.1 MB/s
>
> vs.
>
> root@debian-armhf:~# dd if=/dev/ubiblock0_0 of=/dev/zero bs=1M
> 243+1 records in
> 243+1 records out
> 255080448 bytes (255 MB) copied, 2.87676 s, 88.7 MB/s
>
> Cc: hch@infradead.org
> Cc: axboe@fb.com
> Cc: tom.leiming@gmail.com
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
And thanks a lot for the good work!
--
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2 v2] UBI: Block: Add blk-mq support
@ 2015-01-27 23:37 ` Ezequiel Garcia
0 siblings, 0 replies; 32+ messages in thread
From: Ezequiel Garcia @ 2015-01-27 23:37 UTC (permalink / raw)
To: Richard Weinberger, dedekind1
Cc: axboe, tom.leiming, linux-kernel, hch, linux-mtd,
computersforpeace, dwmw2
On 01/10/2015 06:52 PM, Richard Weinberger wrote:
> Convert the driver to blk-mq.
> Beside of moving to the modern block interface this change boosts
> also the performance of the driver.
>
> nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
> nand: Micron NAND 256MiB 3,3V 8-bit
> nand: 256MiB, SLC, page size: 2048, OOB size: 64
>
> root@debian-armhf:~# dd if=/dev/ubiblock0_0 of=/dev/zero bs=1M
> 243+1 records in
> 243+1 records out
> 255080448 bytes (255 MB) copied, 4.39295 s, 58.1 MB/s
>
> vs.
>
> root@debian-armhf:~# dd if=/dev/ubiblock0_0 of=/dev/zero bs=1M
> 243+1 records in
> 243+1 records out
> 255080448 bytes (255 MB) copied, 2.87676 s, 88.7 MB/s
>
> Cc: hch@infradead.org
> Cc: axboe@fb.com
> Cc: tom.leiming@gmail.com
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Acked-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
And thanks a lot for the good work!
--
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2 v2] UBI: Add initial support for scatter gather
2015-01-27 23:36 ` Ezequiel Garcia
@ 2015-01-27 23:46 ` Richard Weinberger
-1 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-27 23:46 UTC (permalink / raw)
To: Ezequiel Garcia, dedekind1
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel
Am 28.01.2015 um 00:36 schrieb Ezequiel Garcia:
> On 01/10/2015 06:52 PM, Richard Weinberger wrote:
>> Adds a new set of functions to deal with scatter gather.
>> ubi_eba_read_leb_sg() will read from a LEB into a scatter gather list.
>> The new data structure struct ubi_sgl will be used within UBI to
>> hold the scatter gather list itself and metadata to have a cursor
>> within the list.
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Three nitpicks below, and my
>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
>> ---
>> drivers/mtd/ubi/eba.c | 56 +++++++++++++++++++++++++++++
>> drivers/mtd/ubi/kapi.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
>> drivers/mtd/ubi/ubi.h | 3 ++
>> include/linux/mtd/ubi.h | 46 ++++++++++++++++++++++++
>> 4 files changed, 185 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
>> index b698534..0aaf707 100644
>> --- a/drivers/mtd/ubi/eba.c
>> +++ b/drivers/mtd/ubi/eba.c
>> @@ -481,6 +481,62 @@ out_unlock:
>> }
>>
>> /**
>> + * ubi_eba_read_leb_sg - read data into a scatter gather list.
>> + * @ubi: UBI device description object
>> + * @vol: volume description object
>> + * @lnum: logical eraseblock number
>> + * @sgl: UBI scatter gather list to store the read data
>> + * @offset: offset from where to read
>> + * @len: how many bytes to read
>> + * @check: data CRC check flag
>> + *
>> + * This function works exactly like ubi_eba_read_leb(). But instead of
>> + * storing the read data into a buffer it writes to an UBI scatter gather
>> + * list.
>> + */
>> +int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
>> + struct ubi_sgl *sgl, int lnum, int offset, int len,
>> + int check)
>> +{
>> + int to_read;
>> + int ret;
>> + struct scatterlist *sg;
>> +
>> + for (;;) {
>> + ubi_assert(sgl->list_pos < UBI_MAX_SG_COUNT);
>> + sg = &sgl->sg[sgl->list_pos];
>> + if (len < sg->length - sgl->page_pos)
>> + to_read = len;
>> + else
>> + to_read = sg->length - sgl->page_pos;
>> +
>> + ret = ubi_eba_read_leb(ubi, vol, lnum,
>> + sg_virt(sg) + sgl->page_pos, offset,
>> + to_read, check);
>> + if (ret < 0)
>> + goto out;
>
> Matter of taste, but isn't it better to just return here?
After dealing with a lot of nasty bugs in error paths in other projects I've started
using the principle of a single function exit path.
But in this case a "return ret;" is also perfectly fine as the "out" label does no cleanup.
Will fix before pushing.
>> +
>> + offset += to_read;
>> + len -= to_read;
>> + if (!len) {
>> + sgl->page_pos += to_read;
>> + if (sgl->page_pos == sg->length) {
>> + sgl->list_pos++;
>> + sgl->page_pos = 0;
>> + }
>> +
>> + break;
>> + }
>> +
>> + sgl->list_pos++;
>> + sgl->page_pos = 0;
>> + }
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +/**
>> * recover_peb - recover from write failure.
>> * @ubi: UBI device description object
>> * @pnum: the physical eraseblock to recover
>> diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
>> index f3bab66..d0055a2 100644
>> --- a/drivers/mtd/ubi/kapi.c
>> +++ b/drivers/mtd/ubi/kapi.c
>> @@ -355,6 +355,43 @@ void ubi_close_volume(struct ubi_volume_desc *desc)
>> EXPORT_SYMBOL_GPL(ubi_close_volume);
>>
>> /**
>> + * leb_read_sanity_check - does sanity checks on read requests.
>> + * @desc: volume descriptor
>> + * @lnum: logical eraseblock number to read from
>> + * @offset: offset within the logical eraseblock to read from
>> + * @len: how many bytes to read
>> + *
>> + * This function is used by ubi_leb_read() and ubi_leb_read_sg()
>> + * to perfrom sanity checks.
>
> s/perfrom/perform
Thx.
>
>> + */
>> +static int leb_read_sanity_check(struct ubi_volume_desc *desc, int lnum,
>> + int offset, int len)
>> +{
>> + struct ubi_volume *vol = desc->vol;
>> + struct ubi_device *ubi = vol->ubi;
>> + int vol_id = vol->vol_id;
>> +
>> + if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
>> + lnum >= vol->used_ebs || offset < 0 || len < 0 ||
>> + offset + len > vol->usable_leb_size)
>> + return -EINVAL;
>> +
>> + if (vol->vol_type == UBI_STATIC_VOLUME) {
>> + if (vol->used_ebs == 0)
>> + /* Empty static UBI volume */
>> + return 0;
>> + if (lnum == vol->used_ebs - 1 &&
>> + offset + len > vol->last_eb_bytes)
>> + return -EINVAL;
>> + }
>> +
>> + if (vol->upd_marker)
>> + return -EBADF;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> * ubi_leb_read - read data.
>> * @desc: volume descriptor
>> * @lnum: logical eraseblock number to read from
>> @@ -390,22 +427,10 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
>>
>> dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
>>
>> - if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
>> - lnum >= vol->used_ebs || offset < 0 || len < 0 ||
>> - offset + len > vol->usable_leb_size)
>> - return -EINVAL;
>> -
>> - if (vol->vol_type == UBI_STATIC_VOLUME) {
>> - if (vol->used_ebs == 0)
>> - /* Empty static UBI volume */
>> - return 0;
>> - if (lnum == vol->used_ebs - 1 &&
>> - offset + len > vol->last_eb_bytes)
>> - return -EINVAL;
>> - }
>> + err = leb_read_sanity_check(desc, lnum, offset, len);
>> + if (err < 0)
>> + return err;
>>
>> - if (vol->upd_marker)
>> - return -EBADF;
>> if (len == 0)
>> return 0;
>>
>> @@ -419,6 +444,46 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
>> }
>> EXPORT_SYMBOL_GPL(ubi_leb_read);
>>
>> +
>> +/**
>> + * ubi_leb_read_sg - read data into a scatter gather list.
>> + * @desc: volume descriptor
>> + * @lnum: logical eraseblock number to read from
>> + * @buf: buffer where to store the read data
>> + * @offset: offset within the logical eraseblock to read from
>> + * @len: how many bytes to read
>> + * @check: whether UBI has to check the read data's CRC or not.
>> + *
>> + * This function works exactly like ubi_leb_read_sg(). But instead of
>> + * storing the read data into a buffer it writes to an UBI scatter gather
>> + * list.
>> + */
>> +int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
>> + int offset, int len, int check)
>> +{
>> + struct ubi_volume *vol = desc->vol;
>> + struct ubi_device *ubi = vol->ubi;
>> + int err, vol_id = vol->vol_id;
>> +
>> + dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
>> +
>> + err = leb_read_sanity_check(desc, lnum, offset, len);
>> + if (err < 0)
>> + return err;
>> +
>> + if (len == 0)
>> + return 0;
>> +
>> + err = ubi_eba_read_leb_sg(ubi, vol, sgl, lnum, offset, len, check);
>> + if (err && mtd_is_eccerr(err) && vol->vol_type == UBI_STATIC_VOLUME) {
>> + ubi_warn(ubi, "mark volume %d as corrupted", vol_id);
>> + vol->corrupted = 1;
>> + }
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(ubi_leb_read_sg);
>> +
>> /**
>> * ubi_leb_write - write data.
>> * @desc: volume descriptor
>> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
>> index ee7ac0b..a5283cf 100644
>> --- a/drivers/mtd/ubi/ubi.h
>> +++ b/drivers/mtd/ubi/ubi.h
>> @@ -791,6 +791,9 @@ int ubi_eba_unmap_leb(struct ubi_device *ubi, struct ubi_volume *vol,
>> int lnum);
>> int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
>> void *buf, int offset, int len, int check);
>> +int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
>> + struct ubi_sgl *sgl, int lnum, int offset, int len,
>> + int check);
>> int ubi_eba_write_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
>> const void *buf, int offset, int len);
>> int ubi_eba_write_leb_st(struct ubi_device *ubi, struct ubi_volume *vol,
>> diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
>> index c3918a0..3d5916d 100644
>> --- a/include/linux/mtd/ubi.h
>> +++ b/include/linux/mtd/ubi.h
>> @@ -23,11 +23,16 @@
>>
>> #include <linux/ioctl.h>
>> #include <linux/types.h>
>> +#include <linux/scatterlist.h>
>> #include <mtd/ubi-user.h>
>>
>> /* All voumes/LEBs */
>> #define UBI_ALL -1
>>
>> +/* Maximum number of scatter gather list entries,
>> + * we use only 64 to have a lower memory foot print. */
>
> Multilines comments style is:
>
> /*
> *
> */
Correct. I wonder why checkpatch.pl did not bark on that.
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2 v2] UBI: Add initial support for scatter gather
@ 2015-01-27 23:46 ` Richard Weinberger
0 siblings, 0 replies; 32+ messages in thread
From: Richard Weinberger @ 2015-01-27 23:46 UTC (permalink / raw)
To: Ezequiel Garcia, dedekind1
Cc: linux-mtd, computersforpeace, dwmw2, linux-kernel
Am 28.01.2015 um 00:36 schrieb Ezequiel Garcia:
> On 01/10/2015 06:52 PM, Richard Weinberger wrote:
>> Adds a new set of functions to deal with scatter gather.
>> ubi_eba_read_leb_sg() will read from a LEB into a scatter gather list.
>> The new data structure struct ubi_sgl will be used within UBI to
>> hold the scatter gather list itself and metadata to have a cursor
>> within the list.
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Three nitpicks below, and my
>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
>> ---
>> drivers/mtd/ubi/eba.c | 56 +++++++++++++++++++++++++++++
>> drivers/mtd/ubi/kapi.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
>> drivers/mtd/ubi/ubi.h | 3 ++
>> include/linux/mtd/ubi.h | 46 ++++++++++++++++++++++++
>> 4 files changed, 185 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
>> index b698534..0aaf707 100644
>> --- a/drivers/mtd/ubi/eba.c
>> +++ b/drivers/mtd/ubi/eba.c
>> @@ -481,6 +481,62 @@ out_unlock:
>> }
>>
>> /**
>> + * ubi_eba_read_leb_sg - read data into a scatter gather list.
>> + * @ubi: UBI device description object
>> + * @vol: volume description object
>> + * @lnum: logical eraseblock number
>> + * @sgl: UBI scatter gather list to store the read data
>> + * @offset: offset from where to read
>> + * @len: how many bytes to read
>> + * @check: data CRC check flag
>> + *
>> + * This function works exactly like ubi_eba_read_leb(). But instead of
>> + * storing the read data into a buffer it writes to an UBI scatter gather
>> + * list.
>> + */
>> +int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
>> + struct ubi_sgl *sgl, int lnum, int offset, int len,
>> + int check)
>> +{
>> + int to_read;
>> + int ret;
>> + struct scatterlist *sg;
>> +
>> + for (;;) {
>> + ubi_assert(sgl->list_pos < UBI_MAX_SG_COUNT);
>> + sg = &sgl->sg[sgl->list_pos];
>> + if (len < sg->length - sgl->page_pos)
>> + to_read = len;
>> + else
>> + to_read = sg->length - sgl->page_pos;
>> +
>> + ret = ubi_eba_read_leb(ubi, vol, lnum,
>> + sg_virt(sg) + sgl->page_pos, offset,
>> + to_read, check);
>> + if (ret < 0)
>> + goto out;
>
> Matter of taste, but isn't it better to just return here?
After dealing with a lot of nasty bugs in error paths in other projects I've started
using the principle of a single function exit path.
But in this case a "return ret;" is also perfectly fine as the "out" label does no cleanup.
Will fix before pushing.
>> +
>> + offset += to_read;
>> + len -= to_read;
>> + if (!len) {
>> + sgl->page_pos += to_read;
>> + if (sgl->page_pos == sg->length) {
>> + sgl->list_pos++;
>> + sgl->page_pos = 0;
>> + }
>> +
>> + break;
>> + }
>> +
>> + sgl->list_pos++;
>> + sgl->page_pos = 0;
>> + }
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +/**
>> * recover_peb - recover from write failure.
>> * @ubi: UBI device description object
>> * @pnum: the physical eraseblock to recover
>> diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c
>> index f3bab66..d0055a2 100644
>> --- a/drivers/mtd/ubi/kapi.c
>> +++ b/drivers/mtd/ubi/kapi.c
>> @@ -355,6 +355,43 @@ void ubi_close_volume(struct ubi_volume_desc *desc)
>> EXPORT_SYMBOL_GPL(ubi_close_volume);
>>
>> /**
>> + * leb_read_sanity_check - does sanity checks on read requests.
>> + * @desc: volume descriptor
>> + * @lnum: logical eraseblock number to read from
>> + * @offset: offset within the logical eraseblock to read from
>> + * @len: how many bytes to read
>> + *
>> + * This function is used by ubi_leb_read() and ubi_leb_read_sg()
>> + * to perfrom sanity checks.
>
> s/perfrom/perform
Thx.
>
>> + */
>> +static int leb_read_sanity_check(struct ubi_volume_desc *desc, int lnum,
>> + int offset, int len)
>> +{
>> + struct ubi_volume *vol = desc->vol;
>> + struct ubi_device *ubi = vol->ubi;
>> + int vol_id = vol->vol_id;
>> +
>> + if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
>> + lnum >= vol->used_ebs || offset < 0 || len < 0 ||
>> + offset + len > vol->usable_leb_size)
>> + return -EINVAL;
>> +
>> + if (vol->vol_type == UBI_STATIC_VOLUME) {
>> + if (vol->used_ebs == 0)
>> + /* Empty static UBI volume */
>> + return 0;
>> + if (lnum == vol->used_ebs - 1 &&
>> + offset + len > vol->last_eb_bytes)
>> + return -EINVAL;
>> + }
>> +
>> + if (vol->upd_marker)
>> + return -EBADF;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> * ubi_leb_read - read data.
>> * @desc: volume descriptor
>> * @lnum: logical eraseblock number to read from
>> @@ -390,22 +427,10 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
>>
>> dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
>>
>> - if (vol_id < 0 || vol_id >= ubi->vtbl_slots || lnum < 0 ||
>> - lnum >= vol->used_ebs || offset < 0 || len < 0 ||
>> - offset + len > vol->usable_leb_size)
>> - return -EINVAL;
>> -
>> - if (vol->vol_type == UBI_STATIC_VOLUME) {
>> - if (vol->used_ebs == 0)
>> - /* Empty static UBI volume */
>> - return 0;
>> - if (lnum == vol->used_ebs - 1 &&
>> - offset + len > vol->last_eb_bytes)
>> - return -EINVAL;
>> - }
>> + err = leb_read_sanity_check(desc, lnum, offset, len);
>> + if (err < 0)
>> + return err;
>>
>> - if (vol->upd_marker)
>> - return -EBADF;
>> if (len == 0)
>> return 0;
>>
>> @@ -419,6 +444,46 @@ int ubi_leb_read(struct ubi_volume_desc *desc, int lnum, char *buf, int offset,
>> }
>> EXPORT_SYMBOL_GPL(ubi_leb_read);
>>
>> +
>> +/**
>> + * ubi_leb_read_sg - read data into a scatter gather list.
>> + * @desc: volume descriptor
>> + * @lnum: logical eraseblock number to read from
>> + * @buf: buffer where to store the read data
>> + * @offset: offset within the logical eraseblock to read from
>> + * @len: how many bytes to read
>> + * @check: whether UBI has to check the read data's CRC or not.
>> + *
>> + * This function works exactly like ubi_leb_read_sg(). But instead of
>> + * storing the read data into a buffer it writes to an UBI scatter gather
>> + * list.
>> + */
>> +int ubi_leb_read_sg(struct ubi_volume_desc *desc, int lnum, struct ubi_sgl *sgl,
>> + int offset, int len, int check)
>> +{
>> + struct ubi_volume *vol = desc->vol;
>> + struct ubi_device *ubi = vol->ubi;
>> + int err, vol_id = vol->vol_id;
>> +
>> + dbg_gen("read %d bytes from LEB %d:%d:%d", len, vol_id, lnum, offset);
>> +
>> + err = leb_read_sanity_check(desc, lnum, offset, len);
>> + if (err < 0)
>> + return err;
>> +
>> + if (len == 0)
>> + return 0;
>> +
>> + err = ubi_eba_read_leb_sg(ubi, vol, sgl, lnum, offset, len, check);
>> + if (err && mtd_is_eccerr(err) && vol->vol_type == UBI_STATIC_VOLUME) {
>> + ubi_warn(ubi, "mark volume %d as corrupted", vol_id);
>> + vol->corrupted = 1;
>> + }
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(ubi_leb_read_sg);
>> +
>> /**
>> * ubi_leb_write - write data.
>> * @desc: volume descriptor
>> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
>> index ee7ac0b..a5283cf 100644
>> --- a/drivers/mtd/ubi/ubi.h
>> +++ b/drivers/mtd/ubi/ubi.h
>> @@ -791,6 +791,9 @@ int ubi_eba_unmap_leb(struct ubi_device *ubi, struct ubi_volume *vol,
>> int lnum);
>> int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
>> void *buf, int offset, int len, int check);
>> +int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
>> + struct ubi_sgl *sgl, int lnum, int offset, int len,
>> + int check);
>> int ubi_eba_write_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
>> const void *buf, int offset, int len);
>> int ubi_eba_write_leb_st(struct ubi_device *ubi, struct ubi_volume *vol,
>> diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
>> index c3918a0..3d5916d 100644
>> --- a/include/linux/mtd/ubi.h
>> +++ b/include/linux/mtd/ubi.h
>> @@ -23,11 +23,16 @@
>>
>> #include <linux/ioctl.h>
>> #include <linux/types.h>
>> +#include <linux/scatterlist.h>
>> #include <mtd/ubi-user.h>
>>
>> /* All voumes/LEBs */
>> #define UBI_ALL -1
>>
>> +/* Maximum number of scatter gather list entries,
>> + * we use only 64 to have a lower memory foot print. */
>
> Multilines comments style is:
>
> /*
> *
> */
Correct. I wonder why checkpatch.pl did not bark on that.
Thanks,
//richard
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2015-01-27 23:47 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-10 21:52 [PATCH 1/2 v2] UBI: Add initial support for scatter gather Richard Weinberger
2015-01-10 21:52 ` Richard Weinberger
2015-01-10 21:52 ` [PATCH 2/2 v2] UBI: Block: Add blk-mq support Richard Weinberger
2015-01-10 21:52 ` Richard Weinberger
2015-01-13 16:25 ` Christoph Hellwig
2015-01-13 16:25 ` Christoph Hellwig
2015-01-13 22:51 ` Richard Weinberger
2015-01-13 22:51 ` Richard Weinberger
2015-01-13 22:54 ` Jens Axboe
2015-01-13 22:54 ` Jens Axboe
2015-01-13 23:17 ` Richard Weinberger
2015-01-13 23:17 ` Richard Weinberger
2015-01-13 23:30 ` Jens Axboe
2015-01-13 23:30 ` Jens Axboe
2015-01-13 23:36 ` Richard Weinberger
2015-01-13 23:36 ` Richard Weinberger
2015-01-14 0:23 ` Jens Axboe
2015-01-14 0:23 ` Jens Axboe
2015-01-14 8:39 ` Richard Weinberger
2015-01-14 8:39 ` Richard Weinberger
2015-01-26 23:55 ` Richard Weinberger
2015-01-26 23:55 ` Richard Weinberger
2015-01-27 4:03 ` Jens Axboe
2015-01-27 4:03 ` Jens Axboe
2015-01-27 23:37 ` Ezequiel Garcia
2015-01-27 23:37 ` Ezequiel Garcia
2015-01-26 23:53 ` [PATCH 1/2 v2] UBI: Add initial support for scatter gather Richard Weinberger
2015-01-26 23:53 ` Richard Weinberger
2015-01-27 23:36 ` Ezequiel Garcia
2015-01-27 23:36 ` Ezequiel Garcia
2015-01-27 23:46 ` Richard Weinberger
2015-01-27 23:46 ` Richard Weinberger
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.