[PATCH 0/3] introduce sg_pcopy_from_buffer() and sg_pcopy_to_buffer()

[PATCH 0/3] introduce sg_pcopy_from_buffer() and sg_pcopy_to_buffer()

[Akinobu Mita]

This patch set introduces sg_pcopy_from_buffer() and sg_pcopy_to_buffer(),
which copy data between a linear buffer and an SG list.

The only difference between sg_pcopy_{from,to}_buffer() and
sg_copy_{from,to}_buffer() is an additional argument that specifies
the number of bytes to skip the SG list before copying.

The main reason for introducing these functions is to fix a problem
in scsi_debug module.  And there is a local function in crypto/talitos
module, which can be replaced by sg_pcopy_to_buffer().

Akinobu Mita (3):
  lib/scatterlist: introduce sg_pcopy_from_buffer() and
    sg_pcopy_to_buffer()
  crypto: talitos: use sg_pcopy_to_buffer()
  scsi_debug: fix do_device_access() with wrap around range

 drivers/crypto/talitos.c    |  60 +-----------------------
 drivers/scsi/scsi_debug.c   |  43 ++++++++++++++---
 include/linux/scatterlist.h |   5 ++
 lib/scatterlist.c           | 109 ++++++++++++++++++++++++++++++++++++--------
 4 files changed, 131 insertions(+), 86 deletions(-)

Cc: Tejun Heo <tj@kernel.org>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org

-- 
1.8.1.4

[PATCH 1/3] lib/scatterlist: introduce sg_pcopy_from_buffer() and sg_pcopy_to_buffer()

[Akinobu Mita]

The only difference between sg_pcopy_{from,to}_buffer() and
sg_copy_{from,to}_buffer() is an additional argument that specifies
the number of bytes to skip the SG list before copying.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org
---
 include/linux/scatterlist.h |   5 ++
 lib/scatterlist.c           | 109 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 20 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 5951e3f..f5dee42 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -241,6 +241,11 @@ size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
 size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
 			 void *buf, size_t buflen);
 
+size_t sg_pcopy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+			    void *buf, size_t buflen, off_t skip);
+size_t sg_pcopy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+			  void *buf, size_t buflen, off_t skip);
+
 /*
  * Maximum number of entries that will be allocated in one piece, if
  * a list larger than this is required then chaining will be utilized.
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index a1cf8ca..3b40b72 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -453,6 +453,47 @@ void sg_miter_start(struct sg_mapping_iter *miter, struct scatterlist *sgl,
 }
 EXPORT_SYMBOL(sg_miter_start);
 
+static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
+{
+	if (!miter->__remaining) {
+		struct scatterlist *sg;
+		unsigned long pgoffset;
+
+		if (!__sg_page_iter_next(&miter->piter))
+			return false;
+
+		sg = miter->piter.sg;
+		pgoffset = miter->piter.sg_pgoffset;
+
+		miter->__offset = pgoffset ? 0 : sg->offset;
+		miter->__remaining = sg->offset + sg->length -
+				(pgoffset << PAGE_SHIFT) - miter->__offset;
+		miter->__remaining = min_t(unsigned long, miter->__remaining,
+					   PAGE_SIZE - miter->__offset);
+	}
+
+	return true;
+}
+
+static bool sg_miter_seek(struct sg_mapping_iter *miter, off_t offset)
+{
+	WARN_ON(miter->addr);
+
+	while (offset) {
+		off_t consumed;
+
+		if (!sg_miter_get_next_page(miter))
+			return false;
+
+		consumed = min_t(off_t, offset, miter->__remaining);
+		miter->__offset += consumed;
+		miter->__remaining -= consumed;
+		offset -= consumed;
+	}
+
+	return true;
+}
+
 /**
  * sg_miter_next - proceed mapping iterator to the next mapping
  * @miter: sg mapping iter to proceed
@@ -478,22 +519,9 @@ bool sg_miter_next(struct sg_mapping_iter *miter)
 	 * Get to the next page if necessary.
 	 * __remaining, __offset is adjusted by sg_miter_stop
 	 */
-	if (!miter->__remaining) {
-		struct scatterlist *sg;
-		unsigned long pgoffset;
-
-		if (!__sg_page_iter_next(&miter->piter))
-			return false;
-
-		sg = miter->piter.sg;
-		pgoffset = miter->piter.sg_pgoffset;
+	if (!sg_miter_get_next_page(miter))
+		return false;
 
-		miter->__offset = pgoffset ? 0 : sg->offset;
-		miter->__remaining = sg->offset + sg->length -
-				(pgoffset << PAGE_SHIFT) - miter->__offset;
-		miter->__remaining = min_t(unsigned long, miter->__remaining,
-					   PAGE_SIZE - miter->__offset);
-	}
 	miter->page = sg_page_iter_page(&miter->piter);
 	miter->consumed = miter->length = miter->__remaining;
 
@@ -552,14 +580,16 @@ EXPORT_SYMBOL(sg_miter_stop);
  * @nents:		 Number of SG entries
  * @buf:		 Where to copy from
  * @buflen:		 The number of bytes to copy
- * @to_buffer: 		 transfer direction (non zero == from an sg list to a
- * 			 buffer, 0 == from a buffer to an sg list
+ * @skip:		 Number of bytes to skip before copying
+ * @to_buffer:		 transfer direction (true == from an sg list to a
+ *			 buffer, false == from a buffer to an sg list
  *
  * Returns the number of copied bytes.
  *
  **/
 static size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents,
-			     void *buf, size_t buflen, int to_buffer)
+			     void *buf, size_t buflen, off_t skip,
+			     bool to_buffer)
 {
 	unsigned int offset = 0;
 	struct sg_mapping_iter miter;
@@ -573,6 +603,9 @@ static size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents,
 
 	sg_miter_start(&miter, sgl, nents, sg_flags);
 
+	if (!sg_miter_seek(&miter, skip))
+		return false;
+
 	local_irq_save(flags);
 
 	while (sg_miter_next(&miter) && offset < buflen) {
@@ -607,7 +640,7 @@ static size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents,
 size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
 			   void *buf, size_t buflen)
 {
-	return sg_copy_buffer(sgl, nents, buf, buflen, 0);
+	return sg_copy_buffer(sgl, nents, buf, buflen, 0, false);
 }
 EXPORT_SYMBOL(sg_copy_from_buffer);
 
@@ -624,6 +657,42 @@ EXPORT_SYMBOL(sg_copy_from_buffer);
 size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
 			 void *buf, size_t buflen)
 {
-	return sg_copy_buffer(sgl, nents, buf, buflen, 1);
+	return sg_copy_buffer(sgl, nents, buf, buflen, 0, true);
 }
 EXPORT_SYMBOL(sg_copy_to_buffer);
+
+/**
+ * sg_pcopy_from_buffer - Copy from a linear buffer to an SG list
+ * @sgl:		 The SG list
+ * @nents:		 Number of SG entries
+ * @buf:		 Where to copy from
+ * @skip:		 Number of bytes to skip before copying
+ * @buflen:		 The number of bytes to copy
+ *
+ * Returns the number of copied bytes.
+ *
+ **/
+size_t sg_pcopy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+			    void *buf, size_t buflen, off_t skip)
+{
+	return sg_copy_buffer(sgl, nents, buf, buflen, skip, false);
+}
+EXPORT_SYMBOL(sg_pcopy_from_buffer);
+
+/**
+ * sg_pcopy_to_buffer - Copy from an SG list to a linear buffer
+ * @sgl:		 The SG list
+ * @nents:		 Number of SG entries
+ * @buf:		 Where to copy to
+ * @skip:		 Number of bytes to skip before copying
+ * @buflen:		 The number of bytes to copy
+ *
+ * Returns the number of copied bytes.
+ *
+ **/
+size_t sg_pcopy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+			  void *buf, size_t buflen, off_t skip)
+{
+	return sg_copy_buffer(sgl, nents, buf, buflen, skip, true);
+}
+EXPORT_SYMBOL(sg_pcopy_to_buffer);
-- 
1.8.1.4

[PATCH 3/3] scsi_debug: fix do_device_access() with wrap around range

[Akinobu Mita]

do_device_access() is a function that abstracts copying SG list from/to
ramdisk storage (fake_storep).

It must deal with the ranges exceeding actual fake_storep size, because
such ranges are valid if virtual_gb is set greater than zero, and they
should be treated as fake_storep is repeatedly mirrored up to virtual size.

Unfortunately, it can't deal with the range which wraps around the end of
fake_storep. A wrap around range is copied by two sg_copy_{from,to}_buffer()
calls, but sg_copy_{from,to}_buffer() can't copy from/to in the middle of
SG list, therefore the second call can't copy correctly.

This fixes it by using sg_pcopy_{from,to}_buffer() that can copy from/to
the middle of SG list.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/scsi_debug.c | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 21239b3..c1efaf8 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1614,24 +1614,48 @@ static int check_device_access_params(struct sdebug_dev_info *devi,
 	return 0;
 }
 
+/* Returns number of bytes copied or -1 if error. */
 static int do_device_access(struct scsi_cmnd *scmd,
 			    struct sdebug_dev_info *devi,
 			    unsigned long long lba, unsigned int num, int write)
 {
 	int ret;
 	unsigned long long block, rest = 0;
-	int (*func)(struct scsi_cmnd *, unsigned char *, int);
+	struct scsi_data_buffer *sdb;
+	enum dma_data_direction dir;
+	size_t (*func)(struct scatterlist *, unsigned int, void *, size_t,
+		       off_t);
+
+	if (write) {
+		sdb = scsi_out(scmd);
+		dir = DMA_TO_DEVICE;
+		func = sg_pcopy_to_buffer;
+	} else {
+		sdb = scsi_in(scmd);
+		dir = DMA_FROM_DEVICE;
+		func = sg_pcopy_from_buffer;
+	}
 
-	func = write ? fetch_to_dev_buffer : fill_from_dev_buffer;
+	if (!sdb->length)
+		return 0;
+	if (!(scsi_bidi_cmnd(scmd) || scmd->sc_data_direction == dir))
+		return -1;
 
 	block = do_div(lba, sdebug_store_sectors);
 	if (block + num > sdebug_store_sectors)
 		rest = block + num - sdebug_store_sectors;
 
-	ret = func(scmd, fake_storep + (block * scsi_debug_sector_size),
-		   (num - rest) * scsi_debug_sector_size);
-	if (!ret && rest)
-		ret = func(scmd, fake_storep, rest * scsi_debug_sector_size);
+	ret = func(sdb->table.sgl, sdb->table.nents,
+		   fake_storep + (block * scsi_debug_sector_size),
+		   (num - rest) * scsi_debug_sector_size, 0);
+	if (ret != (num - rest) * scsi_debug_sector_size)
+		return ret;
+
+	if (rest) {
+		ret += func(sdb->table.sgl, sdb->table.nents,
+			    fake_storep, rest * scsi_debug_sector_size,
+			    (num - rest) * scsi_debug_sector_size);
+	}
 
 	return ret;
 }
@@ -1888,7 +1912,12 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
 	read_lock_irqsave(&atomic_rw, iflags);
 	ret = do_device_access(SCpnt, devip, lba, num, 0);
 	read_unlock_irqrestore(&atomic_rw, iflags);
-	return ret;
+	if (ret == -1)
+		return DID_ERROR << 16;
+
+	scsi_in(SCpnt)->resid = scsi_bufflen(SCpnt) - ret;
+
+	return 0;
 }
 
 static void dump_sector(unsigned char *buf, int len)
-- 
1.8.1.4

Re: [PATCH 1/3] lib/scatterlist: introduce sg_pcopy_from_buffer() and sg_pcopy_to_buffer()

[Imre Deak]

On Thu, 2013-06-06 at 21:52 +0900, Akinobu Mita wrote:
> The only difference between sg_pcopy_{from,to}_buffer() and
> sg_copy_{from,to}_buffer() is an additional argument that specifies
> the number of bytes to skip the SG list before copying.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-crypto@vger.kernel.org
> Cc: "James E.J. Bottomley" <JBottomley@parallels.com>
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  include/linux/scatterlist.h |   5 ++
>  lib/scatterlist.c           | 109 ++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 94 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index 5951e3f..f5dee42 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -241,6 +241,11 @@ size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
>  size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
>  			 void *buf, size_t buflen);
>  
> +size_t sg_pcopy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> +			    void *buf, size_t buflen, off_t skip);
> +size_t sg_pcopy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> +			  void *buf, size_t buflen, off_t skip);
> +
>  /*
>   * Maximum number of entries that will be allocated in one piece, if
>   * a list larger than this is required then chaining will be utilized.
> diff --git a/lib/scatterlist.c b/lib/scatterlist.c
> index a1cf8ca..3b40b72 100644
> --- a/lib/scatterlist.c
> +++ b/lib/scatterlist.c
> @@ -453,6 +453,47 @@ void sg_miter_start(struct sg_mapping_iter *miter, struct scatterlist *sgl,
>  }
>  EXPORT_SYMBOL(sg_miter_start);
>  
> +static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
> +{
> +	if (!miter->__remaining) {
> +		struct scatterlist *sg;
> +		unsigned long pgoffset;
> +
> +		if (!__sg_page_iter_next(&miter->piter))
> +			return false;
> +
> +		sg = miter->piter.sg;
> +		pgoffset = miter->piter.sg_pgoffset;
> +
> +		miter->__offset = pgoffset ? 0 : sg->offset;
> +		miter->__remaining = sg->offset + sg->length -
> +				(pgoffset << PAGE_SHIFT) - miter->__offset;
> +		miter->__remaining = min_t(unsigned long, miter->__remaining,
> +					   PAGE_SIZE - miter->__offset);
> +	}
> +
> +	return true;
> +}
> +
> +static bool sg_miter_seek(struct sg_mapping_iter *miter, off_t offset)
> +{
> +	WARN_ON(miter->addr);
> +
> +	while (offset) {
> +		off_t consumed;
> +
> +		if (!sg_miter_get_next_page(miter))
> +			return false;
> +
> +		consumed = min_t(off_t, offset, miter->__remaining);
> +		miter->__offset += consumed;
> +		miter->__remaining -= consumed;
> +		offset -= consumed;
> +	}
> +
> +	return true;
> +}
> +
>  /**
>   * sg_miter_next - proceed mapping iterator to the next mapping
>   * @miter: sg mapping iter to proceed
> @@ -478,22 +519,9 @@ bool sg_miter_next(struct sg_mapping_iter *miter)
>  	 * Get to the next page if necessary.
>  	 * __remaining, __offset is adjusted by sg_miter_stop
>  	 */
> -	if (!miter->__remaining) {
> -		struct scatterlist *sg;
> -		unsigned long pgoffset;
> -
> -		if (!__sg_page_iter_next(&miter->piter))
> -			return false;
> -
> -		sg = miter->piter.sg;
> -		pgoffset = miter->piter.sg_pgoffset;
> +	if (!sg_miter_get_next_page(miter))
> +		return false;
>  
> -		miter->__offset = pgoffset ? 0 : sg->offset;
> -		miter->__remaining = sg->offset + sg->length -
> -				(pgoffset << PAGE_SHIFT) - miter->__offset;
> -		miter->__remaining = min_t(unsigned long, miter->__remaining,
> -					   PAGE_SIZE - miter->__offset);
> -	}
>  	miter->page = sg_page_iter_page(&miter->piter);
>  	miter->consumed = miter->length = miter->__remaining;
>  
> @@ -552,14 +580,16 @@ EXPORT_SYMBOL(sg_miter_stop);
>   * @nents:		 Number of SG entries
>   * @buf:		 Where to copy from
>   * @buflen:		 The number of bytes to copy
> - * @to_buffer: 		 transfer direction (non zero == from an sg list to a
> - * 			 buffer, 0 == from a buffer to an sg list
> + * @skip:		 Number of bytes to skip before copying
> + * @to_buffer:		 transfer direction (true == from an sg list to a
> + *			 buffer, false == from a buffer to an sg list
>   *
>   * Returns the number of copied bytes.
>   *
>   **/
>  static size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents,
> -			     void *buf, size_t buflen, int to_buffer)
> +			     void *buf, size_t buflen, off_t skip,
> +			     bool to_buffer)
>  {
>  	unsigned int offset = 0;
>  	struct sg_mapping_iter miter;
> @@ -573,6 +603,9 @@ static size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents,
>  
>  	sg_miter_start(&miter, sgl, nents, sg_flags);
>  
> +	if (!sg_miter_seek(&miter, skip))
> +		return false;

Looks ok to me, perhaps adding the seek functionality to the mapping
iterator would make things more generic and the mapping iterator more
resemble the page iterator. So we'd have a new sg_miter_start_offset and
call it here something like:

sg_miter_start_offset(&miter, sgl, nents, sg_flags, skip);

Just my 2 cents,
Imre

> +
>  	local_irq_save(flags);
>  
>  	while (sg_miter_next(&miter) && offset < buflen) {
> @@ -607,7 +640,7 @@ static size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents,
>  size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
>  			   void *buf, size_t buflen)
>  {
> -	return sg_copy_buffer(sgl, nents, buf, buflen, 0);
> +	return sg_copy_buffer(sgl, nents, buf, buflen, 0, false);
>  }
>  EXPORT_SYMBOL(sg_copy_from_buffer);
>  
> @@ -624,6 +657,42 @@ EXPORT_SYMBOL(sg_copy_from_buffer);
>  size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
>  			 void *buf, size_t buflen)
>  {
> -	return sg_copy_buffer(sgl, nents, buf, buflen, 1);
> +	return sg_copy_buffer(sgl, nents, buf, buflen, 0, true);
>  }
>  EXPORT_SYMBOL(sg_copy_to_buffer);
> +
> +/**
> + * sg_pcopy_from_buffer - Copy from a linear buffer to an SG list
> + * @sgl:		 The SG list
> + * @nents:		 Number of SG entries
> + * @buf:		 Where to copy from
> + * @skip:		 Number of bytes to skip before copying
> + * @buflen:		 The number of bytes to copy
> + *
> + * Returns the number of copied bytes.
> + *
> + **/
> +size_t sg_pcopy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> +			    void *buf, size_t buflen, off_t skip)
> +{
> +	return sg_copy_buffer(sgl, nents, buf, buflen, skip, false);
> +}
> +EXPORT_SYMBOL(sg_pcopy_from_buffer);
> +
> +/**
> + * sg_pcopy_to_buffer - Copy from an SG list to a linear buffer
> + * @sgl:		 The SG list
> + * @nents:		 Number of SG entries
> + * @buf:		 Where to copy to
> + * @skip:		 Number of bytes to skip before copying
> + * @buflen:		 The number of bytes to copy
> + *
> + * Returns the number of copied bytes.
> + *
> + **/
> +size_t sg_pcopy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> +			  void *buf, size_t buflen, off_t skip)
> +{
> +	return sg_copy_buffer(sgl, nents, buf, buflen, skip, true);
> +}
> +EXPORT_SYMBOL(sg_pcopy_to_buffer);

Re: [PATCH 1/3] lib/scatterlist: introduce sg_pcopy_from_buffer() and sg_pcopy_to_buffer()

[Tejun Heo]

Hello,

On Thu, Jun 06, 2013 at 09:52:56PM +0900, Akinobu Mita wrote:
> +static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
> +{
> +	if (!miter->__remaining) {
> +		struct scatterlist *sg;
> +		unsigned long pgoffset;
> +
> +		if (!__sg_page_iter_next(&miter->piter))
> +			return false;
> +
> +		sg = miter->piter.sg;
> +		pgoffset = miter->piter.sg_pgoffset;
> +
> +		miter->__offset = pgoffset ? 0 : sg->offset;
> +		miter->__remaining = sg->offset + sg->length -
> +				(pgoffset << PAGE_SHIFT) - miter->__offset;
> +		miter->__remaining = min_t(unsigned long, miter->__remaining,
> +					   PAGE_SIZE - miter->__offset);
> +	}
> +
> +	return true;
> +}

It'd be better if separating out this function is a separate patch.
Mixing factoring out something and adding new stuff makes the patch a
bit harder to read.

> +static bool sg_miter_seek(struct sg_mapping_iter *miter, off_t offset)
> +{
> +	WARN_ON(miter->addr);
> +
> +	while (offset) {
> +		off_t consumed;
> +
> +		if (!sg_miter_get_next_page(miter))
> +			return false;
> +
> +		consumed = min_t(off_t, offset, miter->__remaining);
> +		miter->__offset += consumed;
> +		miter->__remaining -= consumed;
> +		offset -= consumed;
> +	}
> +
> +	return true;
> +}

While I think the above should work at the beginning, what if @miter
is in the middle of iteration and __remaining isn't zero?

Looks good to me otherwise.

Thanks.

-- 
tejun

Re: [PATCH 1/3] lib/scatterlist: introduce sg_pcopy_from_buffer() and sg_pcopy_to_buffer()

[Akinobu Mita]

2013/6/6 Imre Deak <imre.deak@intel.com>:
> Looks ok to me, perhaps adding the seek functionality to the mapping
> iterator would make things more generic and the mapping iterator more
> resemble the page iterator. So we'd have a new sg_miter_start_offset and
> call it here something like:
>
> sg_miter_start_offset(&miter, sgl, nents, sg_flags, skip);

I also thought something like this could be a new interface for sg_miter_* API,
but I haven't found any places where it can be used yet.  That's why I made
it a local function and didn't create new interface.

But putting good function comment like other sg_miter_* API for new
local function
is harmless and it helps someone who wants interface like this in the future.
So I'll do so in next version.

Re: [PATCH 1/3] lib/scatterlist: introduce sg_pcopy_from_buffer() and sg_pcopy_to_buffer()

[Akinobu Mita]

2013/6/7 Tejun Heo <tj@kernel.org>:
> Hello,
>
> On Thu, Jun 06, 2013 at 09:52:56PM +0900, Akinobu Mita wrote:
>> +static bool sg_miter_get_next_page(struct sg_mapping_iter *miter)
>> +{
>> +     if (!miter->__remaining) {
>> +             struct scatterlist *sg;
>> +             unsigned long pgoffset;
>> +
>> +             if (!__sg_page_iter_next(&miter->piter))
>> +                     return false;
>> +
>> +             sg = miter->piter.sg;
>> +             pgoffset = miter->piter.sg_pgoffset;
>> +
>> +             miter->__offset = pgoffset ? 0 : sg->offset;
>> +             miter->__remaining = sg->offset + sg->length -
>> +                             (pgoffset << PAGE_SHIFT) - miter->__offset;
>> +             miter->__remaining = min_t(unsigned long, miter->__remaining,
>> +                                        PAGE_SIZE - miter->__offset);
>> +     }
>> +
>> +     return true;
>> +}
>
> It'd be better if separating out this function is a separate patch.
> Mixing factoring out something and adding new stuff makes the patch a
> bit harder to read.

OK, sounds good. I'll do so in next version.

But I feel sg_miter_get_next_page() is not very appropriate name, because
it gets the next page only if necessary.  If I can find more appropriate name,
I'll rename it.

>> +static bool sg_miter_seek(struct sg_mapping_iter *miter, off_t offset)
>> +{
>> +     WARN_ON(miter->addr);
>> +
>> +     while (offset) {
>> +             off_t consumed;
>> +
>> +             if (!sg_miter_get_next_page(miter))
>> +                     return false;
>> +
>> +             consumed = min_t(off_t, offset, miter->__remaining);
>> +             miter->__offset += consumed;
>> +             miter->__remaining -= consumed;
>> +             offset -= consumed;
>> +     }
>> +
>> +     return true;
>> +}
>
> While I think the above should work at the beginning, what if @miter
> is in the middle of iteration and __remaining isn't zero?

sg_miter_seek() should work as far as it is called just after sg_miter_start(),
or just after sg_miter_stop() in the middle of iteration (Although I only
tested the former case).  I omitted a function comment in excuse of the static
function, but I should write something I said above.