Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1553020 > unrolled thread

[PATCH] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

Back to article view | Back to linux.kernel

Contents

  [PATCH] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths Jeff Layton <jlayton@redhat.com> - 2017-01-06 19:30 +0100
    Re: [PATCH] ceph/iov_iter: fix bad iov_iter handling in ceph splice  codepaths Jeff Layton <jlayton@redhat.com> - 2017-01-06 21:40 +0100
    Re: [PATCH] ceph/iov_iter: fix bad iov_iter handling in ceph splice  codepaths Jeff Layton <jlayton@redhat.com> - 2017-01-10 00:20 +0100
    [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths Jeff Layton <jlayton@redhat.com> - 2017-01-10 14:00 +0100
      Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths "Yan, Zheng" <zyan@redhat.com> - 2017-01-11 03:50 +0100
      Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Al Viro <viro@ZenIV.linux.org.uk> - 2017-01-12 09:10 +0100
        Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Ilya Dryomov <idryomov@gmail.com> - 2017-01-12 12:20 +0100
          Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Al Viro <viro@ZenIV.linux.org.uk> - 2017-01-12 12:40 +0100
            Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Ilya Dryomov <idryomov@gmail.com> - 2017-01-12 12:50 +0100
              Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Jeff Layton <jlayton@redhat.com> - 2017-01-12 13:00 +0100
              Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Al Viro <viro@ZenIV.linux.org.uk> - 2017-01-12 13:00 +0100
                Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Ilya Dryomov <idryomov@gmail.com> - 2017-01-12 13:20 +0100
        Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Jeff Layton <jlayton@redhat.com> - 2017-01-12 12:30 +0100
          Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph  splice codepaths Ilya Dryomov <idryomov@gmail.com> - 2017-01-12 12:40 +0100

#1553020 — [PATCH] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
fio to drive some I/O via vmsplice ane splice. Ceph then ends trying to
access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it to
pick up a wrong offset and get stuck in an infinite loop while trying to
populate the page array.

To fix this, add a new iov_iter helper for to determine the offset into
the page for the current segment and have ceph call that.

dio_get_pagev_size has similar problems, so fix that as well by adding a
new helper function in lib/iov_iter.c that does what it does, but for
all iov_iter types.

Since we're moving that into generic code, we can also utilize the
iterate_all_kinds macro to handle this. That means that we need to
rework the logic a bit since we can't advance to the next vector while
checking the current one.

Link: http://tracker.ceph.com/issues/18130
Cc: "Zhu, Caifeng" <zhucaifeng@unissoft-nj.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/file.c      | 28 ++------------------
 include/linux/uio.h |  2 ++
 lib/iov_iter.c      | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 26 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index f633165f3fdc..0cd9fc68a04e 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -35,29 +35,6 @@
  */
 
 /*
- * Calculate the length sum of direct io vectors that can
- * be combined into one page vector.
- */
-static size_t dio_get_pagev_size(const struct iov_iter *it)
-{
-    const struct iovec *iov = it->iov;
-    const struct iovec *iovend = iov + it->nr_segs;
-    size_t size;
-
-    size = iov->iov_len - it->iov_offset;
-    /*
-     * An iov can be page vectored when both the current tail
-     * and the next base are page aligned.
-     */
-    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
-           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
-        size += iov->iov_len;
-    }
-    dout("dio_get_pagevlen len = %zu\n", size);
-    return size;
-}
-
-/*
  * Allocate a page vector based on (@it, @nbytes).
  * The return value is the tuple describing a page vector,
  * that is (@pages, @page_align, @num_pages).
@@ -71,8 +48,7 @@ dio_get_pages_alloc(const struct iov_iter *it, size_t nbytes,
 	struct page **pages;
 	int ret = 0, idx, npages;
 
-	align = (unsigned long)(it->iov->iov_base + it->iov_offset) &
-		(PAGE_SIZE - 1);
+	align = iov_iter_single_seg_page_offset(it);
 	npages = calc_pages_for(align, nbytes);
 	pages = kmalloc(sizeof(*pages) * npages, GFP_KERNEL);
 	if (!pages) {
@@ -927,7 +903,7 @@ ceph_direct_read_write(struct kiocb *iocb, struct iov_iter *iter,
 	}
 
 	while (iov_iter_count(iter) > 0) {
-		u64 size = dio_get_pagev_size(iter);
+		u64 size = iov_iter_pvec_size(iter);
 		size_t start = 0;
 		ssize_t len;
 
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 6e22b544d039..46fdfff3d7d6 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -83,6 +83,7 @@ size_t iov_iter_copy_from_user_atomic(struct page *page,
 void iov_iter_advance(struct iov_iter *i, size_t bytes);
 int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes);
 size_t iov_iter_single_seg_count(const struct iov_iter *i);
+size_t iov_iter_single_seg_page_offset(const struct iov_iter *i);
 size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
 			 struct iov_iter *i);
 size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
@@ -93,6 +94,7 @@ size_t copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i);
 size_t iov_iter_zero(size_t bytes, struct iov_iter *);
 unsigned long iov_iter_alignment(const struct iov_iter *i);
 unsigned long iov_iter_gap_alignment(const struct iov_iter *i);
+size_t iov_iter_pvec_size(const struct iov_iter *i);
 void iov_iter_init(struct iov_iter *i, int direction, const struct iovec *iov,
 			unsigned long nr_segs, size_t count);
 void iov_iter_kvec(struct iov_iter *i, int direction, const struct kvec *kvec,
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 6b415b5a100d..d22850bbb1e9 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -745,6 +745,24 @@ size_t iov_iter_single_seg_count(const struct iov_iter *i)
 }
 EXPORT_SYMBOL(iov_iter_single_seg_count);
 
+/* Return offset into page for current iov_iter segment */
+size_t iov_iter_single_seg_page_offset(const struct iov_iter *i)
+{
+	size_t	base;
+
+	if (i->type & ITER_PIPE)
+		base = i->pipe->bufs[i->idx].offset;
+	else if (i->type & ITER_BVEC)
+		base = i->bvec->bv_offset;
+	else if (i->type & ITER_KVEC)
+		base = (size_t)i->kvec->iov_base;
+	else
+		base = (size_t)i->iov->iov_base;
+
+	return (base + i->iov_offset) & (PAGE_SIZE - 1);
+}
+EXPORT_SYMBOL(iov_iter_single_seg_page_offset);
+
 void iov_iter_kvec(struct iov_iter *i, int direction,
 			const struct kvec *kvec, unsigned long nr_segs,
 			size_t count)
@@ -830,6 +848,61 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
 }
 EXPORT_SYMBOL(iov_iter_gap_alignment);
 
+/**
+ * iov_iter_pvec_size - find length of page aligned iovecs in iov_iter
+ * @i: iov_iter to in which to find the size
+ *
+ * Some filesystems can stitch together multiple iovecs into a single
+ * page vector when both the previous tail and current base are page
+ * aligned. This function discovers the length that can fit in a single
+ * pagevec and returns it.
+ */
+size_t iov_iter_pvec_size(const struct iov_iter *i)
+{
+	size_t size = i->count;
+	size_t pv_size = 0;
+	bool contig = false, first = true;
+
+	if (!size)
+		return 0;
+
+	/* Pipes are naturally aligned for this */
+	if (unlikely(i->type & ITER_PIPE))
+		return size;
+
+	/*
+	 * An iov can be page vectored when the current base and previous
+	 * tail are both page aligned. Note that we don't require that the
+	 * initial base in the first iovec also be page aligned.
+	 */
+	iterate_all_kinds(i, size, v,
+		({
+		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
+			pv_size += v.iov_len;
+			first = false;
+			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
+		 }; 0;
+		 }),
+		({
+		 if (first || (contig && v.bv_offset == 0)) {
+			pv_size += v.bv_len;
+			first = false;
+			contig = PAGE_ALIGNED(v.bv_offset + v.bv_len);
+		 }
+		 }),
+		({
+		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
+			pv_size += v.iov_len;
+			first = false;
+			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
+		 }
+		 }))
+
+	pv_size -= i->iov_offset;
+	return pv_size;
+}
+EXPORT_SYMBOL(iov_iter_pvec_size);
+
 static inline size_t __pipe_get_pages(struct iov_iter *i,
 				size_t maxsize,
 				struct page **pages,
-- 
2.7.4

[toc] | [next] | [standalone]

#1553095 — Re: [PATCH] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Fri, 2017-01-06 at 13:23 -0500, Jeff Layton wrote:
> xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
> fio to drive some I/O via vmsplice ane splice. Ceph then ends trying to
> access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it to
> pick up a wrong offset and get stuck in an infinite loop while trying to
> populate the page array.
> 
> To fix this, add a new iov_iter helper for to determine the offset into
> the page for the current segment and have ceph call that.
> 
> dio_get_pagev_size has similar problems, so fix that as well by adding a
> new helper function in lib/iov_iter.c that does what it does, but for
> all iov_iter types.
> 
> Since we're moving that into generic code, we can also utilize the
> iterate_all_kinds macro to handle this. That means that we need to
> rework the logic a bit since we can't advance to the next vector while
> checking the current one.
> 
> Link: http://tracker.ceph.com/issues/18130
> Cc: "Zhu, Caifeng" <zhucaifeng@unissoft-nj.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/ceph/file.c      | 28 ++------------------
>  include/linux/uio.h |  2 ++
>  lib/iov_iter.c      | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index f633165f3fdc..0cd9fc68a04e 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -35,29 +35,6 @@
>   */
>  
>  /*
> - * Calculate the length sum of direct io vectors that can
> - * be combined into one page vector.
> - */
> -static size_t dio_get_pagev_size(const struct iov_iter *it)
> -{
> -    const struct iovec *iov = it->iov;
> -    const struct iovec *iovend = iov + it->nr_segs;
> -    size_t size;
> -
> -    size = iov->iov_len - it->iov_offset;
> -    /*
> -     * An iov can be page vectored when both the current tail
> -     * and the next base are page aligned.
> -     */
> -    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
> -           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
> -        size += iov->iov_len;
> -    }
> -    dout("dio_get_pagevlen len = %zu\n", size);
> -    return size;
> -}
> -
> -/*
>   * Allocate a page vector based on (@it, @nbytes).
>   * The return value is the tuple describing a page vector,
>   * that is (@pages, @page_align, @num_pages).
> @@ -71,8 +48,7 @@ dio_get_pages_alloc(const struct iov_iter *it, size_t nbytes,
>  	struct page **pages;
>  	int ret = 0, idx, npages;
>  
> -	align = (unsigned long)(it->iov->iov_base + it->iov_offset) &
> -		(PAGE_SIZE - 1);
> +	align = iov_iter_single_seg_page_offset(it);
>  	npages = calc_pages_for(align, nbytes);
>  	pages = kmalloc(sizeof(*pages) * npages, GFP_KERNEL);
>  	if (!pages) {
> @@ -927,7 +903,7 @@ ceph_direct_read_write(struct kiocb *iocb, struct iov_iter *iter,
>  	}
>  
>  	while (iov_iter_count(iter) > 0) {
> -		u64 size = dio_get_pagev_size(iter);
> +		u64 size = iov_iter_pvec_size(iter);
>  		size_t start = 0;
>  		ssize_t len;
>  
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 6e22b544d039..46fdfff3d7d6 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -83,6 +83,7 @@ size_t iov_iter_copy_from_user_atomic(struct page *page,
>  void iov_iter_advance(struct iov_iter *i, size_t bytes);
>  int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes);
>  size_t iov_iter_single_seg_count(const struct iov_iter *i);
> +size_t iov_iter_single_seg_page_offset(const struct iov_iter *i);
>  size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
>  			 struct iov_iter *i);
>  size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
> @@ -93,6 +94,7 @@ size_t copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i);
>  size_t iov_iter_zero(size_t bytes, struct iov_iter *);
>  unsigned long iov_iter_alignment(const struct iov_iter *i);
>  unsigned long iov_iter_gap_alignment(const struct iov_iter *i);
> +size_t iov_iter_pvec_size(const struct iov_iter *i);
>  void iov_iter_init(struct iov_iter *i, int direction, const struct iovec *iov,
>  			unsigned long nr_segs, size_t count);
>  void iov_iter_kvec(struct iov_iter *i, int direction, const struct kvec *kvec,
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 6b415b5a100d..d22850bbb1e9 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -745,6 +745,24 @@ size_t iov_iter_single_seg_count(const struct iov_iter *i)
>  }
>  EXPORT_SYMBOL(iov_iter_single_seg_count);
>  
> +/* Return offset into page for current iov_iter segment */
> +size_t iov_iter_single_seg_page_offset(const struct iov_iter *i)
> +{
> +	size_t	base;
> +
> +	if (i->type & ITER_PIPE)
> +		base = i->pipe->bufs[i->idx].offset;
> +	else if (i->type & ITER_BVEC)
> +		base = i->bvec->bv_offset;
> +	else if (i->type & ITER_KVEC)
> +		base = (size_t)i->kvec->iov_base;
> +	else
> +		base = (size_t)i->iov->iov_base;
> +
> +	return (base + i->iov_offset) & (PAGE_SIZE - 1);
> +}
> +EXPORT_SYMBOL(iov_iter_single_seg_page_offset);
> +
>  void iov_iter_kvec(struct iov_iter *i, int direction,
>  			const struct kvec *kvec, unsigned long nr_segs,
>  			size_t count)
> @@ -830,6 +848,61 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
>  }
>  EXPORT_SYMBOL(iov_iter_gap_alignment);
>  
> +/**
> + * iov_iter_pvec_size - find length of page aligned iovecs in iov_iter
> + * @i: iov_iter to in which to find the size
> + *
> + * Some filesystems can stitch together multiple iovecs into a single
> + * page vector when both the previous tail and current base are page
> + * aligned. This function discovers the length that can fit in a single
> + * pagevec and returns it.
> + */
> +size_t iov_iter_pvec_size(const struct iov_iter *i)
> +{
> +	size_t size = i->count;
> +	size_t pv_size = 0;
> +	bool contig = false, first = true;
> +
> +	if (!size)
> +		return 0;
> +
> +	/* Pipes are naturally aligned for this */
> +	if (unlikely(i->type & ITER_PIPE))
> +		return size;
> +
> +	/*
> +	 * An iov can be page vectored when the current base and previous
> +	 * tail are both page aligned. Note that we don't require that the
> +	 * initial base in the first iovec also be page aligned.
> +	 */
> +	iterate_all_kinds(i, size, v,
> +		({
> +		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
> +			pv_size += v.iov_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
> +		 }; 0;
> +		 }),
> +		({
> +		 if (first || (contig && v.bv_offset == 0)) {
> +			pv_size += v.bv_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.bv_offset + v.bv_len);
> +		 }
> +		 }),
> +		({
> +		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
> +			pv_size += v.iov_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
> +		 }
> +		 }))
> +
> +	pv_size -= i->iov_offset;
> +	return pv_size;
> +}
> +EXPORT_SYMBOL(iov_iter_pvec_size);
> +
>  static inline size_t __pipe_get_pages(struct iov_iter *i,
>  				size_t maxsize,
>  				struct page **pages,

Ugh, but this breaks generic/113...let me see what's going on there.
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1554791 — Re: [PATCH] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Fri, 2017-01-06 at 13:23 -0500, Jeff Layton wrote:
> xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
> fio to drive some I/O via vmsplice ane splice. Ceph then ends trying to
> access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it to
> pick up a wrong offset and get stuck in an infinite loop while trying to
> populate the page array.
> 
> To fix this, add a new iov_iter helper for to determine the offset into
> the page for the current segment and have ceph call that.
> 
> dio_get_pagev_size has similar problems, so fix that as well by adding a
> new helper function in lib/iov_iter.c that does what it does, but for
> all iov_iter types.
> 
> Since we're moving that into generic code, we can also utilize the
> iterate_all_kinds macro to handle this. That means that we need to
> rework the logic a bit since we can't advance to the next vector while
> checking the current one.
> 
> Link: http://tracker.ceph.com/issues/18130
> Cc: "Zhu, Caifeng" <zhucaifeng@unissoft-nj.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/ceph/file.c      | 28 ++------------------
>  include/linux/uio.h |  2 ++
>  lib/iov_iter.c      | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index f633165f3fdc..0cd9fc68a04e 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -35,29 +35,6 @@
>   */
>  
>  /*
> - * Calculate the length sum of direct io vectors that can
> - * be combined into one page vector.
> - */
> -static size_t dio_get_pagev_size(const struct iov_iter *it)
> -{
> -    const struct iovec *iov = it->iov;
> -    const struct iovec *iovend = iov + it->nr_segs;
> -    size_t size;
> -
> -    size = iov->iov_len - it->iov_offset;
> -    /*
> -     * An iov can be page vectored when both the current tail
> -     * and the next base are page aligned.
> -     */
> -    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
> -           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
> -        size += iov->iov_len;
> -    }
> -    dout("dio_get_pagevlen len = %zu\n", size);
> -    return size;
> -}
> -
> -/*
>   * Allocate a page vector based on (@it, @nbytes).
>   * The return value is the tuple describing a page vector,
>   * that is (@pages, @page_align, @num_pages).
> @@ -71,8 +48,7 @@ dio_get_pages_alloc(const struct iov_iter *it, size_t nbytes,
>  	struct page **pages;
>  	int ret = 0, idx, npages;
>  
> -	align = (unsigned long)(it->iov->iov_base + it->iov_offset) &
> -		(PAGE_SIZE - 1);
> +	align = iov_iter_single_seg_page_offset(it);
>  	npages = calc_pages_for(align, nbytes);
>  	pages = kmalloc(sizeof(*pages) * npages, GFP_KERNEL);
>  	if (!pages) {

Found the bug with the earlier patch, and will be sending an updated one
after some more testing.

That said, I would love to be able to just get rid of ceph's
dio_get_pages_alloc as well and just use iov_iter_get_pages_alloc
instead, but I don't think we can because the bio_vec iterator block
does this:

                /* can't be more than PAGE_SIZE */
                *start = v.bv_offset;
                *pages = p = get_pages_array(1);
                if (!p)
                        return -ENOMEM;
                get_page(*p = v.bv_page);
                return v.bv_len;

...so what happens here is that iter_file_splice_write creates an
ITER_BVEC iov_iter. Then any filesystem driver that uses
iov_iter_get_pages or iov_iter_get_pages_alloc can only get a single
page for each call. The ceph code works around this by calling
iov_iter_get_pages in a loop until it gets enough, but that's rather
nasty.

I bet this makes splice writing to an O_DIRECT file on NFS suck too...

Why _is_ ITER_BVEC limited to a single page there? I wonder if we could
consider lifting that limitation in some cases if it really is
necessary?

> @@ -927,7 +903,7 @@ ceph_direct_read_write(struct kiocb *iocb, struct iov_iter *iter,
>  	}
>  
>  	while (iov_iter_count(iter) > 0) {
> -		u64 size = dio_get_pagev_size(iter);
> +		u64 size = iov_iter_pvec_size(iter);
>  		size_t start = 0;
>  		ssize_t len;
> 
>  
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 6e22b544d039..46fdfff3d7d6 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -83,6 +83,7 @@ size_t iov_iter_copy_from_user_atomic(struct page *page,
>  void iov_iter_advance(struct iov_iter *i, size_t bytes);
>  int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes);
>  size_t iov_iter_single_seg_count(const struct iov_iter *i);
> +size_t iov_iter_single_seg_page_offset(const struct iov_iter *i);
>  size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
>  			 struct iov_iter *i);
>  size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
> @@ -93,6 +94,7 @@ size_t copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i);
>  size_t iov_iter_zero(size_t bytes, struct iov_iter *);
>  unsigned long iov_iter_alignment(const struct iov_iter *i);
>  unsigned long iov_iter_gap_alignment(const struct iov_iter *i);
> +size_t iov_iter_pvec_size(const struct iov_iter *i);
>  void iov_iter_init(struct iov_iter *i, int direction, const struct iovec *iov,
>  			unsigned long nr_segs, size_t count);
>  void iov_iter_kvec(struct iov_iter *i, int direction, const struct kvec *kvec,
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 6b415b5a100d..d22850bbb1e9 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -745,6 +745,24 @@ size_t iov_iter_single_seg_count(const struct iov_iter *i)
>  }
>  EXPORT_SYMBOL(iov_iter_single_seg_count);
>  
> +/* Return offset into page for current iov_iter segment */
> +size_t iov_iter_single_seg_page_offset(const struct iov_iter *i)
> +{
> +	size_t	base;
> +
> +	if (i->type & ITER_PIPE)
> +		base = i->pipe->bufs[i->idx].offset;
> +	else if (i->type & ITER_BVEC)
> +		base = i->bvec->bv_offset;
> +	else if (i->type & ITER_KVEC)
> +		base = (size_t)i->kvec->iov_base;
> +	else
> +		base = (size_t)i->iov->iov_base;
> +
> +	return (base + i->iov_offset) & (PAGE_SIZE - 1);
> +}
> +EXPORT_SYMBOL(iov_iter_single_seg_page_offset);
> +
>  void iov_iter_kvec(struct iov_iter *i, int direction,
>  			const struct kvec *kvec, unsigned long nr_segs,
>  			size_t count)
> @@ -830,6 +848,61 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
>  }
>  EXPORT_SYMBOL(iov_iter_gap_alignment);
>  
> +/**
> + * iov_iter_pvec_size - find length of page aligned iovecs in iov_iter
> + * @i: iov_iter to in which to find the size
> + *
> + * Some filesystems can stitch together multiple iovecs into a single
> + * page vector when both the previous tail and current base are page
> + * aligned. This function discovers the length that can fit in a single
> + * pagevec and returns it.
> + */
> +size_t iov_iter_pvec_size(const struct iov_iter *i)
> +{
> +	size_t size = i->count;
> +	size_t pv_size = 0;
> +	bool contig = false, first = true;
> +
> +	if (!size)
> +		return 0;
> +
> +	/* Pipes are naturally aligned for this */
> +	if (unlikely(i->type & ITER_PIPE))
> +		return size;
> +
> +	/*
> +	 * An iov can be page vectored when the current base and previous
> +	 * tail are both page aligned. Note that we don't require that the
> +	 * initial base in the first iovec also be page aligned.
> +	 */
> +	iterate_all_kinds(i, size, v,
> +		({
> +		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
> +			pv_size += v.iov_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
> +		 }; 0;
> +		 }),
> +		({
> +		 if (first || (contig && v.bv_offset == 0)) {
> +			pv_size += v.bv_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.bv_offset + v.bv_len);
> +		 }
> +		 }),
> +		({
> +		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
> +			pv_size += v.iov_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
> +		 }
> +		 }))
> +
> +	pv_size -= i->iov_offset;

The bug in this patch is the above. iterate_all_kinds will have already
accounted for the iov_offset, so there's no need to do this here (and it
throws off the calculation when you have to go around more than once).

> +	return pv_size;
> +}
> +EXPORT_SYMBOL(iov_iter_pvec_size);
> +
>  static inline size_t __pipe_get_pages(struct iov_iter *i,
>  				size_t maxsize,
>  				struct page **pages,

-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1555233 — [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

v2: fix bug in offset handling in iov_iter_pvec_size

xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
fio to drive some I/O via vmsplice ane splice. Ceph then ends up trying
to access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it
to pick up a wrong offset and get stuck in an infinite loop while trying
to populate the page array. dio_get_pagev_size has a similar problem.

To fix the first problem, add a new iov_iter helper to determine the
offset into the page for the current segment and have ceph call that.
I would just replace dio_get_pages_alloc with iov_iter_get_pages_alloc,
but that will only return a single page at a time for ITER_BVEC and
it's better to make larger requests when possible.

For the second problem, we simply replace it with a new helper that does
what it does, but properly for all iov_iter types.

Since we're moving that into generic code, we can also utilize the
iterate_all_kinds macro to simplify this. That means that we need to
rework the logic a bit since we can't advance to the next vector while
checking the current one.

Link: http://tracker.ceph.com/issues/18130
Cc: "Zhu, Caifeng" <zhucaifeng@unissoft-nj.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/ceph/file.c      | 28 ++-------------------
 include/linux/uio.h |  2 ++
 lib/iov_iter.c      | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 26 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index f633165f3fdc..0cd9fc68a04e 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -35,29 +35,6 @@
  */
 
 /*
- * Calculate the length sum of direct io vectors that can
- * be combined into one page vector.
- */
-static size_t dio_get_pagev_size(const struct iov_iter *it)
-{
-    const struct iovec *iov = it->iov;
-    const struct iovec *iovend = iov + it->nr_segs;
-    size_t size;
-
-    size = iov->iov_len - it->iov_offset;
-    /*
-     * An iov can be page vectored when both the current tail
-     * and the next base are page aligned.
-     */
-    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
-           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
-        size += iov->iov_len;
-    }
-    dout("dio_get_pagevlen len = %zu\n", size);
-    return size;
-}
-
-/*
  * Allocate a page vector based on (@it, @nbytes).
  * The return value is the tuple describing a page vector,
  * that is (@pages, @page_align, @num_pages).
@@ -71,8 +48,7 @@ dio_get_pages_alloc(const struct iov_iter *it, size_t nbytes,
 	struct page **pages;
 	int ret = 0, idx, npages;
 
-	align = (unsigned long)(it->iov->iov_base + it->iov_offset) &
-		(PAGE_SIZE - 1);
+	align = iov_iter_single_seg_page_offset(it);
 	npages = calc_pages_for(align, nbytes);
 	pages = kmalloc(sizeof(*pages) * npages, GFP_KERNEL);
 	if (!pages) {
@@ -927,7 +903,7 @@ ceph_direct_read_write(struct kiocb *iocb, struct iov_iter *iter,
 	}
 
 	while (iov_iter_count(iter) > 0) {
-		u64 size = dio_get_pagev_size(iter);
+		u64 size = iov_iter_pvec_size(iter);
 		size_t start = 0;
 		ssize_t len;
 
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 6e22b544d039..46fdfff3d7d6 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -83,6 +83,7 @@ size_t iov_iter_copy_from_user_atomic(struct page *page,
 void iov_iter_advance(struct iov_iter *i, size_t bytes);
 int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes);
 size_t iov_iter_single_seg_count(const struct iov_iter *i);
+size_t iov_iter_single_seg_page_offset(const struct iov_iter *i);
 size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
 			 struct iov_iter *i);
 size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
@@ -93,6 +94,7 @@ size_t copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i);
 size_t iov_iter_zero(size_t bytes, struct iov_iter *);
 unsigned long iov_iter_alignment(const struct iov_iter *i);
 unsigned long iov_iter_gap_alignment(const struct iov_iter *i);
+size_t iov_iter_pvec_size(const struct iov_iter *i);
 void iov_iter_init(struct iov_iter *i, int direction, const struct iovec *iov,
 			unsigned long nr_segs, size_t count);
 void iov_iter_kvec(struct iov_iter *i, int direction, const struct kvec *kvec,
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 6b415b5a100d..b8fa377b0cef 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -745,6 +745,24 @@ size_t iov_iter_single_seg_count(const struct iov_iter *i)
 }
 EXPORT_SYMBOL(iov_iter_single_seg_count);
 
+/* Return offset into page for current iov_iter segment */
+size_t iov_iter_single_seg_page_offset(const struct iov_iter *i)
+{
+	size_t	base;
+
+	if (i->type & ITER_PIPE)
+		base = i->pipe->bufs[i->idx].offset;
+	else if (i->type & ITER_BVEC)
+		base = i->bvec->bv_offset;
+	else if (i->type & ITER_KVEC)
+		base = (size_t)i->kvec->iov_base;
+	else
+		base = (size_t)i->iov->iov_base;
+
+	return (base + i->iov_offset) & (PAGE_SIZE - 1);
+}
+EXPORT_SYMBOL(iov_iter_single_seg_page_offset);
+
 void iov_iter_kvec(struct iov_iter *i, int direction,
 			const struct kvec *kvec, unsigned long nr_segs,
 			size_t count)
@@ -830,6 +848,59 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
 }
 EXPORT_SYMBOL(iov_iter_gap_alignment);
 
+/**
+ * iov_iter_pvec_size - find length of page aligned iovecs in iov_iter
+ * @i: iov_iter to in which to find the size
+ *
+ * Some filesystems can stitch together multiple iovecs into a single
+ * page vector when both the previous tail and current base are page
+ * aligned. This function discovers the length that can fit in a single
+ * pagevec and returns it.
+ */
+size_t iov_iter_pvec_size(const struct iov_iter *i)
+{
+	size_t size = i->count;
+	size_t pv_size = 0;
+	bool contig = false, first = true;
+
+	if (!size)
+		return 0;
+
+	/* Pipes are naturally aligned for this */
+	if (unlikely(i->type & ITER_PIPE))
+		return size;
+
+	/*
+	 * An iov can be page vectored when the current base and previous
+	 * tail are both page aligned. Note that we don't require that the
+	 * initial base in the first iovec also be page aligned.
+	 */
+	iterate_all_kinds(i, size, v,
+		({
+		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
+			pv_size += v.iov_len;
+			first = false;
+			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
+		 }; 0;
+		 }),
+		({
+		 if (first || (contig && v.bv_offset == 0)) {
+			pv_size += v.bv_len;
+			first = false;
+			contig = PAGE_ALIGNED(v.bv_offset + v.bv_len);
+		 }
+		 }),
+		({
+		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
+			pv_size += v.iov_len;
+			first = false;
+			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
+		 }
+		 }))
+	return pv_size;
+}
+EXPORT_SYMBOL(iov_iter_pvec_size);
+
 static inline size_t __pipe_get_pages(struct iov_iter *i,
 				size_t maxsize,
 				struct page **pages,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1556172 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

> On 10 Jan 2017, at 20:57, Jeff Layton <jlayton@redhat.com> wrote:
> 
> v2: fix bug in offset handling in iov_iter_pvec_size
> 
> xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
> fio to drive some I/O via vmsplice ane splice. Ceph then ends up trying
> to access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it
> to pick up a wrong offset and get stuck in an infinite loop while trying
> to populate the page array. dio_get_pagev_size has a similar problem.
> 
> To fix the first problem, add a new iov_iter helper to determine the
> offset into the page for the current segment and have ceph call that.
> I would just replace dio_get_pages_alloc with iov_iter_get_pages_alloc,
> but that will only return a single page at a time for ITER_BVEC and
> it's better to make larger requests when possible.
> 
> For the second problem, we simply replace it with a new helper that does
> what it does, but properly for all iov_iter types.
> 
> Since we're moving that into generic code, we can also utilize the
> iterate_all_kinds macro to simplify this. That means that we need to
> rework the logic a bit since we can't advance to the next vector while
> checking the current one.
> 
> Link: http://tracker.ceph.com/issues/18130
> Cc: "Zhu, Caifeng" <zhucaifeng@unissoft-nj.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
> fs/ceph/file.c      | 28 ++-------------------
> include/linux/uio.h |  2 ++
> lib/iov_iter.c      | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 75 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index f633165f3fdc..0cd9fc68a04e 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -35,29 +35,6 @@
>  */
> 
> /*
> - * Calculate the length sum of direct io vectors that can
> - * be combined into one page vector.
> - */
> -static size_t dio_get_pagev_size(const struct iov_iter *it)
> -{
> -    const struct iovec *iov = it->iov;
> -    const struct iovec *iovend = iov + it->nr_segs;
> -    size_t size;
> -
> -    size = iov->iov_len - it->iov_offset;
> -    /*
> -     * An iov can be page vectored when both the current tail
> -     * and the next base are page aligned.
> -     */
> -    while (PAGE_ALIGNED((iov->iov_base + iov->iov_len)) &&
> -           (++iov < iovend && PAGE_ALIGNED((iov->iov_base)))) {
> -        size += iov->iov_len;
> -    }
> -    dout("dio_get_pagevlen len = %zu\n", size);
> -    return size;
> -}
> -
> -/*
>  * Allocate a page vector based on (@it, @nbytes).
>  * The return value is the tuple describing a page vector,
>  * that is (@pages, @page_align, @num_pages).
> @@ -71,8 +48,7 @@ dio_get_pages_alloc(const struct iov_iter *it, size_t nbytes,
> 	struct page **pages;
> 	int ret = 0, idx, npages;
> 
> -	align = (unsigned long)(it->iov->iov_base + it->iov_offset) &
> -		(PAGE_SIZE - 1);
> +	align = iov_iter_single_seg_page_offset(it);
> 	npages = calc_pages_for(align, nbytes);
> 	pages = kmalloc(sizeof(*pages) * npages, GFP_KERNEL);
> 	if (!pages) {
> @@ -927,7 +903,7 @@ ceph_direct_read_write(struct kiocb *iocb, struct iov_iter *iter,
> 	}
> 
> 	while (iov_iter_count(iter) > 0) {
> -		u64 size = dio_get_pagev_size(iter);
> +		u64 size = iov_iter_pvec_size(iter);
> 		size_t start = 0;
> 		ssize_t len;
> 
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 6e22b544d039..46fdfff3d7d6 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -83,6 +83,7 @@ size_t iov_iter_copy_from_user_atomic(struct page *page,
> void iov_iter_advance(struct iov_iter *i, size_t bytes);
> int iov_iter_fault_in_readable(struct iov_iter *i, size_t bytes);
> size_t iov_iter_single_seg_count(const struct iov_iter *i);
> +size_t iov_iter_single_seg_page_offset(const struct iov_iter *i);
> size_t copy_page_to_iter(struct page *page, size_t offset, size_t bytes,
> 			 struct iov_iter *i);
> size_t copy_page_from_iter(struct page *page, size_t offset, size_t bytes,
> @@ -93,6 +94,7 @@ size_t copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i);
> size_t iov_iter_zero(size_t bytes, struct iov_iter *);
> unsigned long iov_iter_alignment(const struct iov_iter *i);
> unsigned long iov_iter_gap_alignment(const struct iov_iter *i);
> +size_t iov_iter_pvec_size(const struct iov_iter *i);
> void iov_iter_init(struct iov_iter *i, int direction, const struct iovec *iov,
> 			unsigned long nr_segs, size_t count);
> void iov_iter_kvec(struct iov_iter *i, int direction, const struct kvec *kvec,
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 6b415b5a100d..b8fa377b0cef 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -745,6 +745,24 @@ size_t iov_iter_single_seg_count(const struct iov_iter *i)
> }
> EXPORT_SYMBOL(iov_iter_single_seg_count);
> 
> +/* Return offset into page for current iov_iter segment */
> +size_t iov_iter_single_seg_page_offset(const struct iov_iter *i)
> +{
> +	size_t	base;
> +
> +	if (i->type & ITER_PIPE)
> +		base = i->pipe->bufs[i->idx].offset;
> +	else if (i->type & ITER_BVEC)
> +		base = i->bvec->bv_offset;
> +	else if (i->type & ITER_KVEC)
> +		base = (size_t)i->kvec->iov_base;
> +	else
> +		base = (size_t)i->iov->iov_base;
> +
> +	return (base + i->iov_offset) & (PAGE_SIZE - 1);
> +}
> +EXPORT_SYMBOL(iov_iter_single_seg_page_offset);
> +
> void iov_iter_kvec(struct iov_iter *i, int direction,
> 			const struct kvec *kvec, unsigned long nr_segs,
> 			size_t count)
> @@ -830,6 +848,59 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
> }
> EXPORT_SYMBOL(iov_iter_gap_alignment);
> 
> +/**
> + * iov_iter_pvec_size - find length of page aligned iovecs in iov_iter
> + * @i: iov_iter to in which to find the size
> + *
> + * Some filesystems can stitch together multiple iovecs into a single
> + * page vector when both the previous tail and current base are page
> + * aligned. This function discovers the length that can fit in a single
> + * pagevec and returns it.
> + */
> +size_t iov_iter_pvec_size(const struct iov_iter *i)
> +{
> +	size_t size = i->count;
> +	size_t pv_size = 0;
> +	bool contig = false, first = true;
> +
> +	if (!size)
> +		return 0;
> +
> +	/* Pipes are naturally aligned for this */
> +	if (unlikely(i->type & ITER_PIPE))
> +		return size;
> +
> +	/*
> +	 * An iov can be page vectored when the current base and previous
> +	 * tail are both page aligned. Note that we don't require that the
> +	 * initial base in the first iovec also be page aligned.
> +	 */
> +	iterate_all_kinds(i, size, v,
> +		({
> +		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
> +			pv_size += v.iov_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
> +		 }; 0;
> +		 }),
> +		({
> +		 if (first || (contig && v.bv_offset == 0)) {
> +			pv_size += v.bv_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.bv_offset + v.bv_len);
> +		 }
> +		 }),
> +		({
> +		 if (first || (contig && PAGE_ALIGNED(v.iov_base))) {
> +			pv_size += v.iov_len;
> +			first = false;
> +			contig = PAGE_ALIGNED(v.iov_base + v.iov_len);
> +		 }
> +		 }))
> +	return pv_size;
> +}
> +EXPORT_SYMBOL(iov_iter_pvec_size);
> +
> static inline size_t __pipe_get_pages(struct iov_iter *i,
> 				size_t maxsize,
> 				struct page **pages,

Reviewed-by: Yan, Zheng <zyan@redhat.com>
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1557166 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Tue, Jan 10, 2017 at 07:57:31AM -0500, Jeff Layton wrote:
> v2: fix bug in offset handling in iov_iter_pvec_size
> 
> xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
> fio to drive some I/O via vmsplice ane splice. Ceph then ends up trying
> to access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it
> to pick up a wrong offset and get stuck in an infinite loop while trying
> to populate the page array. dio_get_pagev_size has a similar problem.
> 
> To fix the first problem, add a new iov_iter helper to determine the
> offset into the page for the current segment and have ceph call that.
> I would just replace dio_get_pages_alloc with iov_iter_get_pages_alloc,
> but that will only return a single page at a time for ITER_BVEC and
> it's better to make larger requests when possible.
> 
> For the second problem, we simply replace it with a new helper that does
> what it does, but properly for all iov_iter types.
> 
> Since we're moving that into generic code, we can also utilize the
> iterate_all_kinds macro to simplify this. That means that we need to
> rework the logic a bit since we can't advance to the next vector while
> checking the current one.

Yecchhh...  That really looks like exposing way too low-level stuff instead
of coming up with saner primitive ;-/

Is page vector + offset in the first page + number of bytes really what
ceph wants?  Would e.g. an array of bio_vec be saner?  Because _that_
would make a lot more natural iov_iter_get_pages_alloc() analogue...

And yes, I realize that you have ->pages wired into the struct ceph_osd_request;
how painful would it be to have it switched to struct bio_vec array instead?

[toc] | [prev] | [next] | [standalone]

#1557374 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Thu, Jan 12, 2017 at 8:59 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Jan 10, 2017 at 07:57:31AM -0500, Jeff Layton wrote:
>> v2: fix bug in offset handling in iov_iter_pvec_size
>>
>> xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
>> fio to drive some I/O via vmsplice ane splice. Ceph then ends up trying
>> to access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it
>> to pick up a wrong offset and get stuck in an infinite loop while trying
>> to populate the page array. dio_get_pagev_size has a similar problem.
>>
>> To fix the first problem, add a new iov_iter helper to determine the
>> offset into the page for the current segment and have ceph call that.
>> I would just replace dio_get_pages_alloc with iov_iter_get_pages_alloc,
>> but that will only return a single page at a time for ITER_BVEC and
>> it's better to make larger requests when possible.
>>
>> For the second problem, we simply replace it with a new helper that does
>> what it does, but properly for all iov_iter types.
>>
>> Since we're moving that into generic code, we can also utilize the
>> iterate_all_kinds macro to simplify this. That means that we need to
>> rework the logic a bit since we can't advance to the next vector while
>> checking the current one.
>
> Yecchhh...  That really looks like exposing way too low-level stuff instead
> of coming up with saner primitive ;-/
>
> Is page vector + offset in the first page + number of bytes really what
> ceph wants?  Would e.g. an array of bio_vec be saner?  Because _that_
> would make a lot more natural iov_iter_get_pages_alloc() analogue...
>
> And yes, I realize that you have ->pages wired into the struct ceph_osd_request;
> how painful would it be to have it switched to struct bio_vec array instead?

It would be a significant and wide-reaching change, but I've been
meaning to look into switching to iov_iter for a couple of releases
now.  There is a lot of ugly code in net/ceph/messenger.c to hangle
iteration over "page vectors", "page lists" and "bio lists".  All of it
predates iov_iter proliferation and is mostly incomplete anyway: IIRC
you can send out of a pagelist but can't recv into a pagelist, etc.

That said, Jeff's patch doesn't look too bad to me...

Thanks,

                Ilya

[toc] | [prev] | [next] | [standalone]

#1557389 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Thu, Jan 12, 2017 at 12:13:31PM +0100, Ilya Dryomov wrote:

> It would be a significant and wide-reaching change, but I've been
> meaning to look into switching to iov_iter for a couple of releases
> now.  There is a lot of ugly code in net/ceph/messenger.c to hangle
> iteration over "page vectors", "page lists" and "bio lists".  All of it
> predates iov_iter proliferation and is mostly incomplete anyway: IIRC
> you can send out of a pagelist but can't recv into a pagelist, etc.

Wait a sec...  Is it done from the same thread that has issued a syscall?
If so, we certainly could just pass iov_iter without bothering with any
form of ..._get_pages(); if not, we'll need at least to get from iovec
to bio_vec, since userland addresses make sense only in the caller's
context...

[toc] | [prev] | [next] | [standalone]

#1557393 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Thu, Jan 12, 2017 at 12:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Jan 12, 2017 at 12:13:31PM +0100, Ilya Dryomov wrote:
>
>> It would be a significant and wide-reaching change, but I've been
>> meaning to look into switching to iov_iter for a couple of releases
>> now.  There is a lot of ugly code in net/ceph/messenger.c to hangle
>> iteration over "page vectors", "page lists" and "bio lists".  All of it
>> predates iov_iter proliferation and is mostly incomplete anyway: IIRC
>> you can send out of a pagelist but can't recv into a pagelist, etc.
>
> Wait a sec...  Is it done from the same thread that has issued a syscall?
> If so, we certainly could just pass iov_iter without bothering with any
> form of ..._get_pages(); if not, we'll need at least to get from iovec
> to bio_vec, since userland addresses make sense only in the caller's
> context...

No, not necessarily - it's also used by rbd (all of net/ceph has two
users: fs/ceph and drivers/block/rbd.c).

Thanks,

                Ilya

[toc] | [prev] | [next] | [standalone]

#1557397 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Thu, 2017-01-12 at 12:46 +0100, Ilya Dryomov wrote:
> On Thu, Jan 12, 2017 at 12:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > 
> > On Thu, Jan 12, 2017 at 12:13:31PM +0100, Ilya Dryomov wrote:
> > 
> > > 
> > > It would be a significant and wide-reaching change, but I've been
> > > meaning to look into switching to iov_iter for a couple of releases
> > > now.  There is a lot of ugly code in net/ceph/messenger.c to hangle
> > > iteration over "page vectors", "page lists" and "bio lists".  All of it
> > > predates iov_iter proliferation and is mostly incomplete anyway: IIRC
> > > you can send out of a pagelist but can't recv into a pagelist, etc.
> > 
> > Wait a sec...  Is it done from the same thread that has issued a syscall?
> > If so, we certainly could just pass iov_iter without bothering with any
> > form of ..._get_pages(); if not, we'll need at least to get from iovec
> > to bio_vec, since userland addresses make sense only in the caller's
> > context...
> 
> No, not necessarily - it's also used by rbd (all of net/ceph has two
> users: fs/ceph and drivers/block/rbd.c).
> 
> 

...and note that the actual send/receive is done from workqueue context
(AFAICT), so we might be operating over the array from a completely
different thread context from where it was submitted. I think we do need
to get page references at the point of submission like this.
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1557398 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Thu, Jan 12, 2017 at 12:46:42PM +0100, Ilya Dryomov wrote:
> On Thu, Jan 12, 2017 at 12:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Thu, Jan 12, 2017 at 12:13:31PM +0100, Ilya Dryomov wrote:
> >
> >> It would be a significant and wide-reaching change, but I've been
> >> meaning to look into switching to iov_iter for a couple of releases
> >> now.  There is a lot of ugly code in net/ceph/messenger.c to hangle
> >> iteration over "page vectors", "page lists" and "bio lists".  All of it
> >> predates iov_iter proliferation and is mostly incomplete anyway: IIRC
> >> you can send out of a pagelist but can't recv into a pagelist, etc.
> >
> > Wait a sec...  Is it done from the same thread that has issued a syscall?
> > If so, we certainly could just pass iov_iter without bothering with any
> > form of ..._get_pages(); if not, we'll need at least to get from iovec
> > to bio_vec, since userland addresses make sense only in the caller's
> > context...
> 
> No, not necessarily - it's also used by rbd (all of net/ceph has two
> users: fs/ceph and drivers/block/rbd.c).

Yes, but rbd doesn't deal with userland-pointing iovecs at all, does it?

[toc] | [prev] | [next] | [standalone]

#1557415 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Thu, Jan 12, 2017 at 12:53 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Jan 12, 2017 at 12:46:42PM +0100, Ilya Dryomov wrote:
>> On Thu, Jan 12, 2017 at 12:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> > On Thu, Jan 12, 2017 at 12:13:31PM +0100, Ilya Dryomov wrote:
>> >
>> >> It would be a significant and wide-reaching change, but I've been
>> >> meaning to look into switching to iov_iter for a couple of releases
>> >> now.  There is a lot of ugly code in net/ceph/messenger.c to hangle
>> >> iteration over "page vectors", "page lists" and "bio lists".  All of it
>> >> predates iov_iter proliferation and is mostly incomplete anyway: IIRC
>> >> you can send out of a pagelist but can't recv into a pagelist, etc.
>> >
>> > Wait a sec...  Is it done from the same thread that has issued a syscall?
>> > If so, we certainly could just pass iov_iter without bothering with any
>> > form of ..._get_pages(); if not, we'll need at least to get from iovec
>> > to bio_vec, since userland addresses make sense only in the caller's
>> > context...
>>
>> No, not necessarily - it's also used by rbd (all of net/ceph has two
>> users: fs/ceph and drivers/block/rbd.c).
>
> Yes, but rbd doesn't deal with userland-pointing iovecs at all, does it?

Correct.  Normal I/O is all bios + currently it uses some of that
custom page vector/list machinery for maintenance operations (rbd
metadata, locks, etc).  No userland pointers whatsoever for rbd, but
the messenger (checksumming + {send,recv}{msg,page}) runs out of the
kworker for both rbd and cephfs.

Thanks,

                Ilya

[toc] | [prev] | [next] | [standalone]

#1557379 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Thu, 2017-01-12 at 07:59 +0000, Al Viro wrote:
> On Tue, Jan 10, 2017 at 07:57:31AM -0500, Jeff Layton wrote:
> > 
> > v2: fix bug in offset handling in iov_iter_pvec_size
> > 
> > xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
> > fio to drive some I/O via vmsplice ane splice. Ceph then ends up trying
> > to access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it
> > to pick up a wrong offset and get stuck in an infinite loop while trying
> > to populate the page array. dio_get_pagev_size has a similar problem.
> > 
> > To fix the first problem, add a new iov_iter helper to determine the
> > offset into the page for the current segment and have ceph call that.
> > I would just replace dio_get_pages_alloc with iov_iter_get_pages_alloc,
> > but that will only return a single page at a time for ITER_BVEC and
> > it's better to make larger requests when possible.
> > 
> > For the second problem, we simply replace it with a new helper that does
> > what it does, but properly for all iov_iter types.
> > 
> > Since we're moving that into generic code, we can also utilize the
> > iterate_all_kinds macro to simplify this. That means that we need to
> > rework the logic a bit since we can't advance to the next vector while
> > checking the current one.
> 
> Yecchhh...  That really looks like exposing way too low-level stuff instead
> of coming up with saner primitive ;-/
> 

Fair point. That said, I'm not terribly thrilled with how
iov_iter_get_pages* works right now.

Note that it only ever touches the first vector. Would it not be better
to keep getting page references if the bvec/iov elements are aligned
properly? It seems quite plausible that they often would be, and being
able to hand back a larger list of pages in most cases would be
advantageous.

IOW, should we have iov_iter_get_pages basically do what
dio_get_pages_alloc does -- try to build as long an array of pages as
possible before returning, provided that the alignment works out?

The NFS DIO code, for instance, could also benefit there. I know we've
had reports there in the past that sending down a bunch of small iovecs
causes a lot of small-sized requests on the wire.

> Is page vector + offset in the first page + number of bytes really what
> ceph wants?  Would e.g. an array of bio_vec be saner?  Because _that_
> would make a lot more natural iov_iter_get_pages_alloc() analogue...
> 
> And yes, I realize that you have ->pages wired into the struct ceph_osd_request;
> how painful would it be to have it switched to struct bio_vec array instead?

Actually...it looks like that might not be too hard. The low-level OSD
handling code can already handle bio_vec arrays in order to service RBD.
It looks like we could switch cephfs to use
osd_req_op_extent_osd_data_bio instead of
osd_req_op_extent_osd_data_pages. That would add a dependency in cephfs
on CONFIG_BLOCK, but I think we could probably live with that.

-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1557388 — Re: [PATCH v2] ceph/iov_iter: fix bad iov_iter handling in ceph splice codepaths

On Thu, Jan 12, 2017 at 12:27 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Thu, 2017-01-12 at 07:59 +0000, Al Viro wrote:
>> On Tue, Jan 10, 2017 at 07:57:31AM -0500, Jeff Layton wrote:
>> >
>> > v2: fix bug in offset handling in iov_iter_pvec_size
>> >
>> > xfstest generic/095 triggers soft lockups in kcephfs. Basically it uses
>> > fio to drive some I/O via vmsplice ane splice. Ceph then ends up trying
>> > to access an ITER_BVEC type iov_iter as a ITER_IOVEC one. That causes it
>> > to pick up a wrong offset and get stuck in an infinite loop while trying
>> > to populate the page array. dio_get_pagev_size has a similar problem.
>> >
>> > To fix the first problem, add a new iov_iter helper to determine the
>> > offset into the page for the current segment and have ceph call that.
>> > I would just replace dio_get_pages_alloc with iov_iter_get_pages_alloc,
>> > but that will only return a single page at a time for ITER_BVEC and
>> > it's better to make larger requests when possible.
>> >
>> > For the second problem, we simply replace it with a new helper that does
>> > what it does, but properly for all iov_iter types.
>> >
>> > Since we're moving that into generic code, we can also utilize the
>> > iterate_all_kinds macro to simplify this. That means that we need to
>> > rework the logic a bit since we can't advance to the next vector while
>> > checking the current one.
>>
>> Yecchhh...  That really looks like exposing way too low-level stuff instead
>> of coming up with saner primitive ;-/
>>
>
> Fair point. That said, I'm not terribly thrilled with how
> iov_iter_get_pages* works right now.
>
> Note that it only ever touches the first vector. Would it not be better
> to keep getting page references if the bvec/iov elements are aligned
> properly? It seems quite plausible that they often would be, and being
> able to hand back a larger list of pages in most cases would be
> advantageous.
>
> IOW, should we have iov_iter_get_pages basically do what
> dio_get_pages_alloc does -- try to build as long an array of pages as
> possible before returning, provided that the alignment works out?
>
> The NFS DIO code, for instance, could also benefit there. I know we've
> had reports there in the past that sending down a bunch of small iovecs
> causes a lot of small-sized requests on the wire.
>
>> Is page vector + offset in the first page + number of bytes really what
>> ceph wants?  Would e.g. an array of bio_vec be saner?  Because _that_
>> would make a lot more natural iov_iter_get_pages_alloc() analogue...
>>
>> And yes, I realize that you have ->pages wired into the struct ceph_osd_request;
>> how painful would it be to have it switched to struct bio_vec array instead?
>
> Actually...it looks like that might not be too hard. The low-level OSD
> handling code can already handle bio_vec arrays in order to service RBD.
> It looks like we could switch cephfs to use
> osd_req_op_extent_osd_data_bio instead of
> osd_req_op_extent_osd_data_pages. That would add a dependency in cephfs
> on CONFIG_BLOCK, but I think we could probably live with that.

Ah, just that part might be easy enough ;)

Thanks,

                Ilya

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web