* [lustre-devel] [PATCH] staging: lustre: o2iblnd: iov fixes for kiblnd_send
@ 2016-07-15 5:01 ` Oleg Drokin
0 siblings, 0 replies; 8+ messages in thread
From: Oleg Drokin @ 2016-07-15 5:01 UTC (permalink / raw)
To: James Simmons
Cc: Al Viro, Andreas Dilger, Linux Kernel Mailing List,
linux-fsdevel, Lustre Development List
On Jul 11, 2016, at 1:18 PM, James Simmons wrote:
> With the move to iov_iter handling two issues merged
> for the ko2iblnd driver. The first fix address a simple
> typo of the wrong flag being used with iov_iter_kvec.
> The second fix adds the payload offset to the payload
> size.
>
> Signed-off-by: James Simmons <jsimmons@infradead.org>
> ---
> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> index 3d597dc..437e149 100644
> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> @@ -1519,12 +1519,15 @@ kiblnd_send(lnet_ni_t *ni, void *private, lnet_msg_t *lntmsg)
> /* payload is either all vaddrs or all pages */
> LASSERT(!(payload_kiov && payload_iov));
>
> - if (payload_kiov)
> + if (payload_kiov) {
The braces are in fact going to make checkpatch complain that we do not
need them here.
> iov_iter_bvec(&from, ITER_BVEC | WRITE,
> - payload_kiov, payload_niov, payload_nob);
> - else
> - iov_iter_kvec(&from, ITER_BVEC | WRITE,
> - payload_iov, payload_niov, payload_nob);
> + payload_kiov, payload_niov,
> + payload_nob + payload_offset);
Why are we adding the offset to number of bytes here?
> + } else {
> + iov_iter_kvec(&from, ITER_KVEC | WRITE,
> + payload_iov, payload_niov,
> + payload_nob + payload_offset);
> + }
> iov_iter_advance(&from, payload_offset);
Ah, I guess we added it there to then subtract here?
Do you mind if I just merge this change into Al's patch with you as another
Signed-off-by line instead?
Since we caught this early, probably no point in having a breakage point in
the history as it might break a future bisect.
> switch (type) {
> --
> 2.7.4
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] staging: lustre: o2iblnd: iov fixes for kiblnd_send
2016-07-15 5:01 ` [lustre-devel] " Oleg Drokin
@ 2016-07-15 5:14 ` Al Viro
-1 siblings, 0 replies; 8+ messages in thread
From: Al Viro @ 2016-07-15 5:14 UTC (permalink / raw)
To: Oleg Drokin
Cc: James Simmons, Andreas Dilger, Linux Kernel Mailing List,
linux-fsdevel, Lustre Development List
On Fri, Jul 15, 2016 at 01:01:35AM -0400, Oleg Drokin wrote:
> > + } else {
> > + iov_iter_kvec(&from, ITER_KVEC | WRITE,
> > + payload_iov, payload_niov,
> > + payload_nob + payload_offset);
> > + }
> > iov_iter_advance(&from, payload_offset);
>
> Ah, I guess we added it there to then subtract here?
Yes (and that actually fixes a real bug there).
> Do you mind if I just merge this change into Al's patch with you as another
> Signed-off-by line instead?
> Since we caught this early, probably no point in having a breakage point in
> the history as it might break a future bisect.
No objections from me. FWIW, I'd rather have all of that go through your
tree - it's independent from everything else in vfs.git and pulls quite
a bit from yours, so...
^ permalink raw reply [flat|nested] 8+ messages in thread* [lustre-devel] [PATCH] staging: lustre: o2iblnd: iov fixes for kiblnd_send
@ 2016-07-15 5:14 ` Al Viro
0 siblings, 0 replies; 8+ messages in thread
From: Al Viro @ 2016-07-15 5:14 UTC (permalink / raw)
To: Oleg Drokin
Cc: James Simmons, Andreas Dilger, Linux Kernel Mailing List,
linux-fsdevel, Lustre Development List
On Fri, Jul 15, 2016 at 01:01:35AM -0400, Oleg Drokin wrote:
> > + } else {
> > + iov_iter_kvec(&from, ITER_KVEC | WRITE,
> > + payload_iov, payload_niov,
> > + payload_nob + payload_offset);
> > + }
> > iov_iter_advance(&from, payload_offset);
>
> Ah, I guess we added it there to then subtract here?
Yes (and that actually fixes a real bug there).
> Do you mind if I just merge this change into Al's patch with you as another
> Signed-off-by line instead?
> Since we caught this early, probably no point in having a breakage point in
> the history as it might break a future bisect.
No objections from me. FWIW, I'd rather have all of that go through your
tree - it's independent from everything else in vfs.git and pulls quite
a bit from yours, so...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] staging: lustre: o2iblnd: iov fixes for kiblnd_send
2016-07-15 5:01 ` [lustre-devel] " Oleg Drokin
@ 2016-07-18 15:23 ` James Simmons
-1 siblings, 0 replies; 8+ messages in thread
From: James Simmons @ 2016-07-18 15:23 UTC (permalink / raw)
To: Oleg Drokin
Cc: Al Viro, Andreas Dilger, Linux Kernel Mailing List,
linux-fsdevel, Lustre Development List
> On Jul 11, 2016, at 1:18 PM, James Simmons wrote:
>
> > With the move to iov_iter handling two issues merged
> > for the ko2iblnd driver. The first fix address a simple
> > typo of the wrong flag being used with iov_iter_kvec.
> > The second fix adds the payload offset to the payload
> > size.
> >
> > Signed-off-by: James Simmons <jsimmons@infradead.org>
> > ---
> > drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> > index 3d597dc..437e149 100644
> > --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> > +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> > @@ -1519,12 +1519,15 @@ kiblnd_send(lnet_ni_t *ni, void *private, lnet_msg_t *lntmsg)
> > /* payload is either all vaddrs or all pages */
> > LASSERT(!(payload_kiov && payload_iov));
> >
> > - if (payload_kiov)
> > + if (payload_kiov) {
>
> The braces are in fact going to make checkpatch complain that we do not
> need them here.
>
> > iov_iter_bvec(&from, ITER_BVEC | WRITE,
> > - payload_kiov, payload_niov, payload_nob);
> > - else
> > - iov_iter_kvec(&from, ITER_BVEC | WRITE,
> > - payload_iov, payload_niov, payload_nob);
> > + payload_kiov, payload_niov,
> > + payload_nob + payload_offset);
>
> Why are we adding the offset to number of bytes here?
>
> > + } else {
> > + iov_iter_kvec(&from, ITER_KVEC | WRITE,
> > + payload_iov, payload_niov,
> > + payload_nob + payload_offset);
> > + }
> > iov_iter_advance(&from, payload_offset);
>
> Ah, I guess we added it there to then subtract here?
>
> Do you mind if I just merge this change into Al's patch with you as another
> Signed-off-by line instead?
Sure, I'm fine with that. All I care is that this is merged.
> Since we caught this early, probably no point in having a breakage point in
> the history as it might break a future bisect.
>
>
> > switch (type) {
> > --
> > 2.7.4
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* [lustre-devel] [PATCH] staging: lustre: o2iblnd: iov fixes for kiblnd_send
@ 2016-07-18 15:23 ` James Simmons
0 siblings, 0 replies; 8+ messages in thread
From: James Simmons @ 2016-07-18 15:23 UTC (permalink / raw)
To: Oleg Drokin
Cc: Al Viro, Andreas Dilger, Linux Kernel Mailing List,
linux-fsdevel, Lustre Development List
> On Jul 11, 2016, at 1:18 PM, James Simmons wrote:
>
> > With the move to iov_iter handling two issues merged
> > for the ko2iblnd driver. The first fix address a simple
> > typo of the wrong flag being used with iov_iter_kvec.
> > The second fix adds the payload offset to the payload
> > size.
> >
> > Signed-off-by: James Simmons <jsimmons@infradead.org>
> > ---
> > drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> > index 3d597dc..437e149 100644
> > --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> > +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> > @@ -1519,12 +1519,15 @@ kiblnd_send(lnet_ni_t *ni, void *private, lnet_msg_t *lntmsg)
> > /* payload is either all vaddrs or all pages */
> > LASSERT(!(payload_kiov && payload_iov));
> >
> > - if (payload_kiov)
> > + if (payload_kiov) {
>
> The braces are in fact going to make checkpatch complain that we do not
> need them here.
>
> > iov_iter_bvec(&from, ITER_BVEC | WRITE,
> > - payload_kiov, payload_niov, payload_nob);
> > - else
> > - iov_iter_kvec(&from, ITER_BVEC | WRITE,
> > - payload_iov, payload_niov, payload_nob);
> > + payload_kiov, payload_niov,
> > + payload_nob + payload_offset);
>
> Why are we adding the offset to number of bytes here?
>
> > + } else {
> > + iov_iter_kvec(&from, ITER_KVEC | WRITE,
> > + payload_iov, payload_niov,
> > + payload_nob + payload_offset);
> > + }
> > iov_iter_advance(&from, payload_offset);
>
> Ah, I guess we added it there to then subtract here?
>
> Do you mind if I just merge this change into Al's patch with you as another
> Signed-off-by line instead?
Sure, I'm fine with that. All I care is that this is merged.
> Since we caught this early, probably no point in having a breakage point in
> the history as it might break a future bisect.
>
>
> > switch (type) {
> > --
> > 2.7.4
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread