All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] splice: sendfile() at once fails for big files
@ 2015-04-23 15:03 Christophe Leroy
  2015-04-27  7:01 ` Herbert Xu
  0 siblings, 1 reply; 10+ messages in thread
From: Christophe Leroy @ 2015-04-23 15:03 UTC (permalink / raw)
  To: Alexander Viro; +Cc: linux-kernel, linux-fsdevel, linux-crypto

Using sendfile with below small program to get MD5 sums of some files,
it appear that big files (over 64kbytes with 4k pages system) get a
wrong MD5 sum while small files get the correct sum.
This program uses sendfile() to send a file to an AF_ALG socket
for hashing.


/* md5sum2.c */
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <fcntl.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <linux/if_alg.h>

int main(int argc, char **argv)
{
	int sk = socket(AF_ALG, SOCK_SEQPACKET, 0);
	struct stat st;
	struct sockaddr_alg sa = {
		.salg_family = AF_ALG,
		.salg_type = "hash",
		.salg_name = "md5",
	};
	int n;

	bind(sk, (struct sockaddr*)&sa, sizeof(sa));

	for (n = 1; n < argc; n++) {
		int size;
		int offset = 0;
		char buf[4096];
		int fd;
		int sko;
		int i;

		fd = open(argv[n], O_RDONLY);
		sko = accept(sk, NULL, 0);
		fstat(fd, &st);
		size = st.st_size;
		sendfile(sko, fd, &offset, size);
		size = read(sko, buf, sizeof(buf));
		for (i = 0; i < size; i++)
			printf("%2.2x", buf[i]);
		printf("  %s\n", argv[n]);
		close(fd);
		close(sko);
	}
	exit(0);
}

Test below is done using official linux patch files. First result is
with a software based md5sum. Second result is with the program above.

root@vgoip:~# ls -l patch-3.6.*
-rw-r--r--    1 root     root         64011 Aug 24 12:01 patch-3.6.2.gz
-rw-r--r--    1 root     root         94131 Aug 24 12:01 patch-3.6.3.gz

root@vgoip:~# md5sum patch-3.6.*
b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz

root@vgoip:~# ./md5sum2 patch-3.6.*
b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
5fd77b24e68bb24dcc72d6e57c64790e  patch-3.6.3.gz


After investivation, it appears that sendfile() sends the files by blocks
of 64kbytes (16 times PAGE_SIZE). The problem is that at the end of each
block, the SPLICE_F_MORE flag is missing, therefore the hashing operation
is reset as if it was the end of the file.

This patch adds SPLICE_F_MORE to the flags when more data is pending.

With the patch applied, we get the correct sums:

root@vgoip:~# md5sum patch-3.6.*
b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz

root@vgoip:~# ./md5sum2 patch-3.6.*
b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 v2: no change, only new commit text
 
 fs/splice.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/splice.c b/fs/splice.c
index 476024b..fe61723 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1161,7 +1161,7 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
 	long ret, bytes;
 	umode_t i_mode;
 	size_t len;
-	int i, flags;
+	int i, flags, more;
 
 	/*
 	 * We require the input being a regular file, as we don't want to
@@ -1204,6 +1204,7 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
 	 * Don't block on output, we have to drain the direct pipe.
 	 */
 	sd->flags &= ~SPLICE_F_NONBLOCK;
+	more = sd->flags & SPLICE_F_MORE;
 
 	while (len) {
 		size_t read_len;
@@ -1216,6 +1217,10 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
 		read_len = ret;
 		sd->total_len = read_len;
 
+		if (read_len < len)
+			sd->flags |= SPLICE_F_MORE;
+		else if (!more)
+			sd->flags &= ~SPLICE_F_MORE;
 		/*
 		 * NOTE: nonblocking mode only applies to the input. We
 		 * must not do the output in nonblocking mode as then we
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
  2015-04-23 15:03 [PATCH v2] splice: sendfile() at once fails for big files Christophe Leroy
@ 2015-04-27  7:01 ` Herbert Xu
  2015-05-06  3:41   ` Linus Torvalds
  0 siblings, 1 reply; 10+ messages in thread
From: Herbert Xu @ 2015-04-27  7:01 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: viro, linux-kernel, linux-fsdevel, linux-crypto, Jens Axboe,
	Linus Torvalds

Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> Using sendfile with below small program to get MD5 sums of some files,
> it appear that big files (over 64kbytes with 4k pages system) get a
> wrong MD5 sum while small files get the correct sum.
> This program uses sendfile() to send a file to an AF_ALG socket
> for hashing.

Jens, any comments on this patch?

> /* md5sum2.c */
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <string.h>
> #include <fcntl.h>
> #include <sys/socket.h>
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <linux/if_alg.h>
> 
> int main(int argc, char **argv)
> {
>        int sk = socket(AF_ALG, SOCK_SEQPACKET, 0);
>        struct stat st;
>        struct sockaddr_alg sa = {
>                .salg_family = AF_ALG,
>                .salg_type = "hash",
>                .salg_name = "md5",
>        };
>        int n;
> 
>        bind(sk, (struct sockaddr*)&sa, sizeof(sa));
> 
>        for (n = 1; n < argc; n++) {
>                int size;
>                int offset = 0;
>                char buf[4096];
>                int fd;
>                int sko;
>                int i;
> 
>                fd = open(argv[n], O_RDONLY);
>                sko = accept(sk, NULL, 0);
>                fstat(fd, &st);
>                size = st.st_size;
>                sendfile(sko, fd, &offset, size);
>                size = read(sko, buf, sizeof(buf));
>                for (i = 0; i < size; i++)
>                        printf("%2.2x", buf[i]);
>                printf("  %s\n", argv[n]);
>                close(fd);
>                close(sko);
>        }
>        exit(0);
> }
> 
> Test below is done using official linux patch files. First result is
> with a software based md5sum. Second result is with the program above.
> 
> root@vgoip:~# ls -l patch-3.6.*
> -rw-r--r--    1 root     root         64011 Aug 24 12:01 patch-3.6.2.gz
> -rw-r--r--    1 root     root         94131 Aug 24 12:01 patch-3.6.3.gz
> 
> root@vgoip:~# md5sum patch-3.6.*
> b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
> c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz
> 
> root@vgoip:~# ./md5sum2 patch-3.6.*
> b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
> 5fd77b24e68bb24dcc72d6e57c64790e  patch-3.6.3.gz
> 
> 
> After investivation, it appears that sendfile() sends the files by blocks
> of 64kbytes (16 times PAGE_SIZE). The problem is that at the end of each
> block, the SPLICE_F_MORE flag is missing, therefore the hashing operation
> is reset as if it was the end of the file.
> 
> This patch adds SPLICE_F_MORE to the flags when more data is pending.
> 
> With the patch applied, we get the correct sums:
> 
> root@vgoip:~# md5sum patch-3.6.*
> b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
> c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz
> 
> root@vgoip:~# ./md5sum2 patch-3.6.*
> b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
> c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> v2: no change, only new commit text
> 
> fs/splice.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/splice.c b/fs/splice.c
> index 476024b..fe61723 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1161,7 +1161,7 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
>        long ret, bytes;
>        umode_t i_mode;
>        size_t len;
> -       int i, flags;
> +       int i, flags, more;
> 
>        /*
>         * We require the input being a regular file, as we don't want to
> @@ -1204,6 +1204,7 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
>         * Don't block on output, we have to drain the direct pipe.
>         */
>        sd->flags &= ~SPLICE_F_NONBLOCK;
> +       more = sd->flags & SPLICE_F_MORE;
> 
>        while (len) {
>                size_t read_len;
> @@ -1216,6 +1217,10 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
>                read_len = ret;
>                sd->total_len = read_len;
> 
> +               if (read_len < len)
> +                       sd->flags |= SPLICE_F_MORE;
> +               else if (!more)
> +                       sd->flags &= ~SPLICE_F_MORE;
>                /*
>                 * NOTE: nonblocking mode only applies to the input. We
>                 * must not do the output in nonblocking mode as then we

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
  2015-04-27  7:01 ` Herbert Xu
@ 2015-05-06  3:41   ` Linus Torvalds
  2015-05-06  4:34     ` Christoph Hellwig
  2015-05-06 14:23     ` Jens Axboe
  0 siblings, 2 replies; 10+ messages in thread
From: Linus Torvalds @ 2015-05-06  3:41 UTC (permalink / raw)
  To: Herbert Xu, Jens Axboe
  Cc: Christophe Leroy, Al Viro, Linux Kernel Mailing List,
	linux-fsdevel, Linux Crypto Mailing List

Jens, ping?

The test results should make this a no-brainer, but I hate how random
these flag ops.

                      Linus


On Mon, Apr 27, 2015 at 12:01 AM, Herbert Xu
<herbert@gondor.apana.org.au> wrote:
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Using sendfile with below small program to get MD5 sums of some files,
>> it appear that big files (over 64kbytes with 4k pages system) get a
>> wrong MD5 sum while small files get the correct sum.
>> This program uses sendfile() to send a file to an AF_ALG socket
>> for hashing.
>
> Jens, any comments on this patch?
>
>> /* md5sum2.c */
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <unistd.h>
>> #include <string.h>
>> #include <fcntl.h>
>> #include <sys/socket.h>
>> #include <sys/stat.h>
>> #include <sys/types.h>
>> #include <linux/if_alg.h>
>>
>> int main(int argc, char **argv)
>> {
>>        int sk = socket(AF_ALG, SOCK_SEQPACKET, 0);
>>        struct stat st;
>>        struct sockaddr_alg sa = {
>>                .salg_family = AF_ALG,
>>                .salg_type = "hash",
>>                .salg_name = "md5",
>>        };
>>        int n;
>>
>>        bind(sk, (struct sockaddr*)&sa, sizeof(sa));
>>
>>        for (n = 1; n < argc; n++) {
>>                int size;
>>                int offset = 0;
>>                char buf[4096];
>>                int fd;
>>                int sko;
>>                int i;
>>
>>                fd = open(argv[n], O_RDONLY);
>>                sko = accept(sk, NULL, 0);
>>                fstat(fd, &st);
>>                size = st.st_size;
>>                sendfile(sko, fd, &offset, size);
>>                size = read(sko, buf, sizeof(buf));
>>                for (i = 0; i < size; i++)
>>                        printf("%2.2x", buf[i]);
>>                printf("  %s\n", argv[n]);
>>                close(fd);
>>                close(sko);
>>        }
>>        exit(0);
>> }
>>
>> Test below is done using official linux patch files. First result is
>> with a software based md5sum. Second result is with the program above.
>>
>> root@vgoip:~# ls -l patch-3.6.*
>> -rw-r--r--    1 root     root         64011 Aug 24 12:01 patch-3.6.2.gz
>> -rw-r--r--    1 root     root         94131 Aug 24 12:01 patch-3.6.3.gz
>>
>> root@vgoip:~# md5sum patch-3.6.*
>> b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
>> c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz
>>
>> root@vgoip:~# ./md5sum2 patch-3.6.*
>> b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
>> 5fd77b24e68bb24dcc72d6e57c64790e  patch-3.6.3.gz
>>
>>
>> After investivation, it appears that sendfile() sends the files by blocks
>> of 64kbytes (16 times PAGE_SIZE). The problem is that at the end of each
>> block, the SPLICE_F_MORE flag is missing, therefore the hashing operation
>> is reset as if it was the end of the file.
>>
>> This patch adds SPLICE_F_MORE to the flags when more data is pending.
>>
>> With the patch applied, we get the correct sums:
>>
>> root@vgoip:~# md5sum patch-3.6.*
>> b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
>> c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz
>>
>> root@vgoip:~# ./md5sum2 patch-3.6.*
>> b3ffb9848196846f31b2ff133d2d6443  patch-3.6.2.gz
>> c5e8f687878457db77cb7158c38a7e43  patch-3.6.3.gz
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> v2: no change, only new commit text
>>
>> fs/splice.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/splice.c b/fs/splice.c
>> index 476024b..fe61723 100644
>> --- a/fs/splice.c
>> +++ b/fs/splice.c
>> @@ -1161,7 +1161,7 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
>>        long ret, bytes;
>>        umode_t i_mode;
>>        size_t len;
>> -       int i, flags;
>> +       int i, flags, more;
>>
>>        /*
>>         * We require the input being a regular file, as we don't want to
>> @@ -1204,6 +1204,7 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
>>         * Don't block on output, we have to drain the direct pipe.
>>         */
>>        sd->flags &= ~SPLICE_F_NONBLOCK;
>> +       more = sd->flags & SPLICE_F_MORE;
>>
>>        while (len) {
>>                size_t read_len;
>> @@ -1216,6 +1217,10 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
>>                read_len = ret;
>>                sd->total_len = read_len;
>>
>> +               if (read_len < len)
>> +                       sd->flags |= SPLICE_F_MORE;
>> +               else if (!more)
>> +                       sd->flags &= ~SPLICE_F_MORE;
>>                /*
>>                 * NOTE: nonblocking mode only applies to the input. We
>>                 * must not do the output in nonblocking mode as then we
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
  2015-05-06  3:41   ` Linus Torvalds
@ 2015-05-06  4:34     ` Christoph Hellwig
  2015-05-06 14:23     ` Jens Axboe
  1 sibling, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2015-05-06  4:34 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Herbert Xu, Jens Axboe, Christophe Leroy, Al Viro,
	Linux Kernel Mailing List, linux-fsdevel,
	Linux Crypto Mailing List

On Tue, May 05, 2015 at 08:41:05PM -0700, Linus Torvalds wrote:
> Jens, ping?
> 
> The test results should make this a no-brainer, but I hate how random
> these flag ops.

Not Jens here, but:

 (a) the patch looks correct, we do want to set the more flag if there
     is more data
 (b) I think  the AF_ALG model is totally broken if it relies on that
     for data integrity purposes.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
  2015-05-06  3:41   ` Linus Torvalds
  2015-05-06  4:34     ` Christoph Hellwig
@ 2015-05-06 14:23     ` Jens Axboe
  2015-05-06 14:38       ` leroy christophe
  1 sibling, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2015-05-06 14:23 UTC (permalink / raw)
  To: Linus Torvalds, Herbert Xu
  Cc: Christophe Leroy, Al Viro, Linux Kernel Mailing List,
	linux-fsdevel, Linux Crypto Mailing List

On 05/05/2015 09:41 PM, Linus Torvalds wrote:
> Jens, ping?
>
> The test results should make this a no-brainer, but I hate how random
> these flag ops.

Missed the original, apparently. I too am confused how this is a 
correctness fix and not just an optimization.

+               if (read_len < len)
+                       sd->flags |= SPLICE_F_MORE;
+               else if (!more)
+                       sd->flags &= ~SPLICE_F_MORE;

Should that check be for 'more', not '!more'?


-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
  2015-05-06 14:23     ` Jens Axboe
@ 2015-05-06 14:38       ` leroy christophe
  2015-05-06 14:40           ` Jens Axboe
  0 siblings, 1 reply; 10+ messages in thread
From: leroy christophe @ 2015-05-06 14:38 UTC (permalink / raw)
  To: Jens Axboe, Linus Torvalds, Herbert Xu
  Cc: Al Viro, Linux Kernel Mailing List, linux-fsdevel,
	Linux Crypto Mailing List


Le 06/05/2015 16:23, Jens Axboe a écrit :
> On 05/05/2015 09:41 PM, Linus Torvalds wrote:
>> Jens, ping?
>>
>> The test results should make this a no-brainer, but I hate how random
>> these flag ops.
>
> Missed the original, apparently. I too am confused how this is a
> correctness fix and not just an optimization.
>
> +               if (read_len < len)
> +                       sd->flags |= SPLICE_F_MORE;
> +               else if (!more)
> +                       sd->flags &= ~SPLICE_F_MORE;
>
> Should that check be for 'more', not '!more'?
>
>
@@ -1204,6 +1204,7 @@ ssize_t splice_direct_to_actor(struct file *in, 
struct splice_desc *sd,
       * Don't block on output, we have to drain the direct pipe.
       */
      sd->flags &= ~SPLICE_F_NONBLOCK;
+    more = sd->flags & SPLICE_F_MORE;

      while (len) {
          size_t read_len;
@@ -1216,6 +1217,10 @@ ssize_t splice_direct_to_actor(struct file *in, 
struct splice_desc *sd,
          read_len = ret;
          sd->total_len = read_len;

+        if (read_len < len)
+            sd->flags |= SPLICE_F_MORE;
+        else if (!more)
+            sd->flags &= ~SPLICE_F_MORE;



'more' contains whether sendfile() has been called with SPLICE_F_MORE or 
not.
Until all bytes are processed, we have to force SPLICE_F_MORE regardless 
of how sendfile() was called.
Once all bytes have been read, we have to reset the flags according to 
how sendfile() was called, so if 'more' is NOT set, we have to clear 
SPLICE_F_MORE from sd->flags (which was unconditionaly set for 
processing the first bytes)

Christophe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
  2015-05-06 14:38       ` leroy christophe
@ 2015-05-06 14:40           ` Jens Axboe
  0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2015-05-06 14:40 UTC (permalink / raw)
  To: leroy christophe, Linus Torvalds, Herbert Xu
  Cc: Al Viro, Linux Kernel Mailing List, linux-fsdevel,
	Linux Crypto Mailing List

On 05/06/2015 08:38 AM, leroy christophe wrote:
>
> Le 06/05/2015 16:23, Jens Axboe a écrit :
>> On 05/05/2015 09:41 PM, Linus Torvalds wrote:
>>> Jens, ping?
>>>
>>> The test results should make this a no-brainer, but I hate how random
>>> these flag ops.
>>
>> Missed the original, apparently. I too am confused how this is a
>> correctness fix and not just an optimization.
>>
>> +               if (read_len < len)
>> +                       sd->flags |= SPLICE_F_MORE;
>> +               else if (!more)
>> +                       sd->flags &= ~SPLICE_F_MORE;
>>
>> Should that check be for 'more', not '!more'?
>>
>>
> @@ -1204,6 +1204,7 @@ ssize_t splice_direct_to_actor(struct file *in,
> struct splice_desc *sd,
>        * Don't block on output, we have to drain the direct pipe.
>        */
>       sd->flags &= ~SPLICE_F_NONBLOCK;
> +    more = sd->flags & SPLICE_F_MORE;
>
>       while (len) {
>           size_t read_len;
> @@ -1216,6 +1217,10 @@ ssize_t splice_direct_to_actor(struct file *in,
> struct splice_desc *sd,
>           read_len = ret;
>           sd->total_len = read_len;
>
> +        if (read_len < len)
> +            sd->flags |= SPLICE_F_MORE;
> +        else if (!more)
> +            sd->flags &= ~SPLICE_F_MORE;
>
>
>
> 'more' contains whether sendfile() has been called with SPLICE_F_MORE or
> not.
> Until all bytes are processed, we have to force SPLICE_F_MORE regardless
> of how sendfile() was called.
> Once all bytes have been read, we have to reset the flags according to
> how sendfile() was called, so if 'more' is NOT set, we have to clear
> SPLICE_F_MORE from sd->flags (which was unconditionaly set for
> processing the first bytes)

Ah gotcha, that looks correct. Patch is fine with me then.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
@ 2015-05-06 14:40           ` Jens Axboe
  0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2015-05-06 14:40 UTC (permalink / raw)
  To: leroy christophe, Linus Torvalds, Herbert Xu
  Cc: Al Viro, Linux Kernel Mailing List, linux-fsdevel,
	Linux Crypto Mailing List

On 05/06/2015 08:38 AM, leroy christophe wrote:
>
> Le 06/05/2015 16:23, Jens Axboe a écrit :
>> On 05/05/2015 09:41 PM, Linus Torvalds wrote:
>>> Jens, ping?
>>>
>>> The test results should make this a no-brainer, but I hate how random
>>> these flag ops.
>>
>> Missed the original, apparently. I too am confused how this is a
>> correctness fix and not just an optimization.
>>
>> +               if (read_len < len)
>> +                       sd->flags |= SPLICE_F_MORE;
>> +               else if (!more)
>> +                       sd->flags &= ~SPLICE_F_MORE;
>>
>> Should that check be for 'more', not '!more'?
>>
>>
> @@ -1204,6 +1204,7 @@ ssize_t splice_direct_to_actor(struct file *in,
> struct splice_desc *sd,
>        * Don't block on output, we have to drain the direct pipe.
>        */
>       sd->flags &= ~SPLICE_F_NONBLOCK;
> +    more = sd->flags & SPLICE_F_MORE;
>
>       while (len) {
>           size_t read_len;
> @@ -1216,6 +1217,10 @@ ssize_t splice_direct_to_actor(struct file *in,
> struct splice_desc *sd,
>           read_len = ret;
>           sd->total_len = read_len;
>
> +        if (read_len < len)
> +            sd->flags |= SPLICE_F_MORE;
> +        else if (!more)
> +            sd->flags &= ~SPLICE_F_MORE;
>
>
>
> 'more' contains whether sendfile() has been called with SPLICE_F_MORE or
> not.
> Until all bytes are processed, we have to force SPLICE_F_MORE regardless
> of how sendfile() was called.
> Once all bytes have been read, we have to reset the flags according to
> how sendfile() was called, so if 'more' is NOT set, we have to clear
> SPLICE_F_MORE from sd->flags (which was unconditionaly set for
> processing the first bytes)

Ah gotcha, that looks correct. Patch is fine with me then.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
  2015-05-06 14:40           ` Jens Axboe
@ 2015-05-06 15:07             ` Jens Axboe
  -1 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2015-05-06 15:07 UTC (permalink / raw)
  To: leroy christophe, Linus Torvalds, Herbert Xu
  Cc: Al Viro, Linux Kernel Mailing List, linux-fsdevel,
	Linux Crypto Mailing List

On 05/06/2015 08:40 AM, Jens Axboe wrote:
> On 05/06/2015 08:38 AM, leroy christophe wrote:
>>
>> Le 06/05/2015 16:23, Jens Axboe a écrit :
>>> On 05/05/2015 09:41 PM, Linus Torvalds wrote:
>>>> Jens, ping?
>>>>
>>>> The test results should make this a no-brainer, but I hate how random
>>>> these flag ops.
>>>
>>> Missed the original, apparently. I too am confused how this is a
>>> correctness fix and not just an optimization.
>>>
>>> +               if (read_len < len)
>>> +                       sd->flags |= SPLICE_F_MORE;
>>> +               else if (!more)
>>> +                       sd->flags &= ~SPLICE_F_MORE;
>>>
>>> Should that check be for 'more', not '!more'?
>>>
>>>
>> @@ -1204,6 +1204,7 @@ ssize_t splice_direct_to_actor(struct file *in,
>> struct splice_desc *sd,
>>        * Don't block on output, we have to drain the direct pipe.
>>        */
>>       sd->flags &= ~SPLICE_F_NONBLOCK;
>> +    more = sd->flags & SPLICE_F_MORE;
>>
>>       while (len) {
>>           size_t read_len;
>> @@ -1216,6 +1217,10 @@ ssize_t splice_direct_to_actor(struct file *in,
>> struct splice_desc *sd,
>>           read_len = ret;
>>           sd->total_len = read_len;
>>
>> +        if (read_len < len)
>> +            sd->flags |= SPLICE_F_MORE;
>> +        else if (!more)
>> +            sd->flags &= ~SPLICE_F_MORE;
>>
>>
>>
>> 'more' contains whether sendfile() has been called with SPLICE_F_MORE or
>> not.
>> Until all bytes are processed, we have to force SPLICE_F_MORE regardless
>> of how sendfile() was called.
>> Once all bytes have been read, we have to reset the flags according to
>> how sendfile() was called, so if 'more' is NOT set, we have to clear
>> SPLICE_F_MORE from sd->flags (which was unconditionaly set for
>> processing the first bytes)
>
> Ah gotcha, that looks correct. Patch is fine with me then.

Needs a comment added to that effect, imho.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] splice: sendfile() at once fails for big files
@ 2015-05-06 15:07             ` Jens Axboe
  0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2015-05-06 15:07 UTC (permalink / raw)
  To: leroy christophe, Linus Torvalds, Herbert Xu
  Cc: Al Viro, Linux Kernel Mailing List, linux-fsdevel,
	Linux Crypto Mailing List

On 05/06/2015 08:40 AM, Jens Axboe wrote:
> On 05/06/2015 08:38 AM, leroy christophe wrote:
>>
>> Le 06/05/2015 16:23, Jens Axboe a écrit :
>>> On 05/05/2015 09:41 PM, Linus Torvalds wrote:
>>>> Jens, ping?
>>>>
>>>> The test results should make this a no-brainer, but I hate how random
>>>> these flag ops.
>>>
>>> Missed the original, apparently. I too am confused how this is a
>>> correctness fix and not just an optimization.
>>>
>>> +               if (read_len < len)
>>> +                       sd->flags |= SPLICE_F_MORE;
>>> +               else if (!more)
>>> +                       sd->flags &= ~SPLICE_F_MORE;
>>>
>>> Should that check be for 'more', not '!more'?
>>>
>>>
>> @@ -1204,6 +1204,7 @@ ssize_t splice_direct_to_actor(struct file *in,
>> struct splice_desc *sd,
>>        * Don't block on output, we have to drain the direct pipe.
>>        */
>>       sd->flags &= ~SPLICE_F_NONBLOCK;
>> +    more = sd->flags & SPLICE_F_MORE;
>>
>>       while (len) {
>>           size_t read_len;
>> @@ -1216,6 +1217,10 @@ ssize_t splice_direct_to_actor(struct file *in,
>> struct splice_desc *sd,
>>           read_len = ret;
>>           sd->total_len = read_len;
>>
>> +        if (read_len < len)
>> +            sd->flags |= SPLICE_F_MORE;
>> +        else if (!more)
>> +            sd->flags &= ~SPLICE_F_MORE;
>>
>>
>>
>> 'more' contains whether sendfile() has been called with SPLICE_F_MORE or
>> not.
>> Until all bytes are processed, we have to force SPLICE_F_MORE regardless
>> of how sendfile() was called.
>> Once all bytes have been read, we have to reset the flags according to
>> how sendfile() was called, so if 'more' is NOT set, we have to clear
>> SPLICE_F_MORE from sd->flags (which was unconditionaly set for
>> processing the first bytes)
>
> Ah gotcha, that looks correct. Patch is fine with me then.

Needs a comment added to that effect, imho.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-05-06 15:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-23 15:03 [PATCH v2] splice: sendfile() at once fails for big files Christophe Leroy
2015-04-27  7:01 ` Herbert Xu
2015-05-06  3:41   ` Linus Torvalds
2015-05-06  4:34     ` Christoph Hellwig
2015-05-06 14:23     ` Jens Axboe
2015-05-06 14:38       ` leroy christophe
2015-05-06 14:40         ` Jens Axboe
2015-05-06 14:40           ` Jens Axboe
2015-05-06 15:07           ` Jens Axboe
2015-05-06 15:07             ` Jens Axboe

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.