[Qemu-devel] [PATCH v3 0/3] Break down the MTP write operation

[Qemu-devel] [PATCH v3 0/3] Break down the MTP write operation

[Bandan Das]

v3:
  Added patch 3/3
v2:
  Rebased on top of master and retested

For larger files, not only do we keep reallocating to increase the mtp buffer
size, the write also happens in one go. This does two things:

Write to file upto a certain data size we have received so far and second,
reuse the buffer again instead of reallocating to a larger buffer size.

Tested with different file sizes on a Linux guest.


Bandan Das (3):
  usb-mtp: Reallocate buffer in multiples of MTP_WRITE_BUF_SZ
  usb-mtp: breakup MTP write into smaller chunks
  usb-mtp: replace the homebrew write with qemu_write_full

 hw/usb/dev-mtp.c | 166 +++++++++++++++++++++++++++++------------------
 1 file changed, 102 insertions(+), 64 deletions(-)

-- 
2.19.2

[Qemu-devel] [PATCH v3 1/3] usb-mtp: Reallocate buffer in multiples of MTP_WRITE_BUF_SZ

[Bandan Das]

This is a "pre-patch" to breaking up the write buffer for
MTP writes. Instead of allocating a mtp buffer equal to size
sent by the initiator, we start with a small size and reallocate
multiples (of that small size) as needed.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 68c5eb8eaa..2f3536a26d 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -152,7 +152,6 @@ struct MTPData {
     bool         first;
     /* Used for >4G file sizes */
     bool         pending;
-    uint64_t     cached_length;
     int          fd;
 };
 
@@ -244,6 +243,7 @@ typedef struct {
 
 #define MTP_MANUFACTURER  "QEMU"
 #define MTP_PRODUCT       "QEMU filesharing"
+#define MTP_WRITE_BUF_SZ  512000
 
 enum {
     STR_MANUFACTURER = 1,
@@ -1658,7 +1658,7 @@ static void usb_mtp_write_data(MTPState *s)
             d->fd = mkdir(path, mask);
             goto free;
         }
-        if ((s->dataset.size != 0xFFFFFFFF) && (s->dataset.size < d->length)) {
+        if ((s->dataset.size != 0xFFFFFFFF) && (s->dataset.size != d->offset)) {
             usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
                                  0, 0, 0, 0);
             goto done;
@@ -1776,17 +1776,21 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         total_len = cpu_to_le32(container->length) - sizeof(mtp_container);
         /* Length of data in this packet */
         data_len -= sizeof(mtp_container);
-        usb_mtp_realloc(d, total_len);
-        d->length += total_len;
+        if (total_len < MTP_WRITE_BUF_SZ) {
+                usb_mtp_realloc(d, total_len);
+                d->length += total_len;
+        } else {
+                usb_mtp_realloc(d, MTP_WRITE_BUF_SZ - sizeof(mtp_container));
+                d->length += MTP_WRITE_BUF_SZ - sizeof(mtp_container);
+        }
         d->offset = 0;
-        d->cached_length = total_len;
         d->first = false;
         d->pending = false;
     }
 
     if (d->pending) {
-        usb_mtp_realloc(d, d->cached_length);
-        d->length += d->cached_length;
+        usb_mtp_realloc(d, MTP_WRITE_BUF_SZ);
+        d->length += MTP_WRITE_BUF_SZ;
         d->pending = false;
     }
 
@@ -1794,12 +1798,6 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         dlen = data_len;
     } else {
         dlen = d->length - d->offset;
-        /* Check for cached data for large files */
-        if ((s->dataset.size == 0xFFFFFFFF) && (dlen < p->iov.size)) {
-            usb_mtp_realloc(d, p->iov.size - dlen);
-            d->length += p->iov.size - dlen;
-            dlen = p->iov.size;
-        }
     }
 
     switch (d->code) {
@@ -1821,7 +1819,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         d->offset += dlen;
         if ((p->iov.size % 64) || !p->iov.size) {
             assert((s->dataset.size == 0xFFFFFFFF) ||
-                   (s->dataset.size == d->length));
+                   (s->dataset.size == d->offset));
 
             usb_mtp_write_data(s);
             usb_mtp_data_free(s->data_out);
-- 
2.19.2

Re: [Qemu-devel] [PATCH v3 1/3] usb-mtp: Reallocate buffer in multiples of MTP_WRITE_BUF_SZ

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

On 1/28/19 8:24 AM, Bandan Das wrote:
> This is a "pre-patch" to breaking up the write buffer for
> MTP writes. Instead of allocating a mtp buffer equal to size
> sent by the initiator, we start with a small size and reallocate
> multiples (of that small size) as needed.
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 68c5eb8eaa..2f3536a26d 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -152,7 +152,6 @@ struct MTPData {
>      bool         first;
>      /* Used for >4G file sizes */
>      bool         pending;
> -    uint64_t     cached_length;
>      int          fd;
>  };
>  
> @@ -244,6 +243,7 @@ typedef struct {
>  
>  #define MTP_MANUFACTURER  "QEMU"
>  #define MTP_PRODUCT       "QEMU filesharing"
> +#define MTP_WRITE_BUF_SZ  512000

Why a non-power-of-2 instead of using units.h and writing (512 * KiB)?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 1/3] usb-mtp: Reallocate buffer in multiples of MTP_WRITE_BUF_SZ

[Bandan Das]

Eric Blake <eblake@redhat.com> writes:

> On 1/28/19 8:24 AM, Bandan Das wrote:
>> This is a "pre-patch" to breaking up the write buffer for
>> MTP writes. Instead of allocating a mtp buffer equal to size
>> sent by the initiator, we start with a small size and reallocate
>> multiples (of that small size) as needed.
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  hw/usb/dev-mtp.c | 26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>> 
>> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
>> index 68c5eb8eaa..2f3536a26d 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -152,7 +152,6 @@ struct MTPData {
>>      bool         first;
>>      /* Used for >4G file sizes */
>>      bool         pending;
>> -    uint64_t     cached_length;
>>      int          fd;
>>  };
>>  
>> @@ -244,6 +243,7 @@ typedef struct {
>>  
>>  #define MTP_MANUFACTURER  "QEMU"
>>  #define MTP_PRODUCT       "QEMU filesharing"
>> +#define MTP_WRITE_BUF_SZ  512000
>
> Why a non-power-of-2 instead of using units.h and writing (512 * KiB)?

Thank you for the reminder, Eric. I posted a v4.

[Qemu-devel] [PATCH v3 2/3] usb-mtp: breakup MTP write into smaller chunks

[Bandan Das]

For every MTP_WRITE_BUF_SZ copied, this patch writes it to file before
getting the next block of data. The file is kept opened for the
duration of the operation but the sanity checks on the write operation
are performed only once when the write operation starts. Additionally,
we also update the file size in the object metadata once the file has
completely been written.

Suggested-by: Gerd Hoffman <kraxel@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 134 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 91 insertions(+), 43 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 2f3536a26d..b607c7be05 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -35,6 +35,13 @@ enum mtp_container_type {
     TYPE_EVENT    = 4,
 };
 
+/* MTP write stage, for internal use only */
+enum mtp_write_status {
+    WRITE_START    = 1,
+    WRITE_CONTINUE = 2,
+    WRITE_END      = 3,
+};
+
 enum mtp_code {
     /* command codes */
     CMD_GET_DEVICE_INFO            = 0x1001,
@@ -153,6 +160,9 @@ struct MTPData {
     /* Used for >4G file sizes */
     bool         pending;
     int          fd;
+    uint8_t      write_status;
+    /* Internal pointer per every MTP_WRITE_BUF_SZ */
+    uint64_t     data_offset;
 };
 
 struct MTPObject {
@@ -1618,10 +1628,14 @@ static char *utf16_to_str(uint8_t len, uint16_t *arr)
 }
 
 /* Wrapper around write, returns 0 on failure */
-static uint64_t write_retry(int fd, void *buf, uint64_t size)
+static uint64_t write_retry(int fd, void *buf, uint64_t size, off_t offset)
 {
         uint64_t bytes_left = size, ret;
 
+        if (lseek(fd, offset, SEEK_SET) < 0) {
+            goto done;
+        }
+
         while (bytes_left > 0) {
                 ret = write(fd, buf, bytes_left);
                 if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
@@ -1632,9 +1646,20 @@ static uint64_t write_retry(int fd, void *buf, uint64_t size)
                 buf += ret;
         }
 
+done:
         return size - bytes_left;
 }
 
+static void usb_mtp_update_object(MTPObject *parent, char *name)
+{
+    MTPObject *o =
+        usb_mtp_object_lookup_name(parent, name, strlen(name));
+
+    if (o) {
+        lstat(o->path, &o->stat);
+    }
+}
+
 static void usb_mtp_write_data(MTPState *s)
 {
     MTPData *d = s->data_out;
@@ -1646,48 +1671,56 @@ static void usb_mtp_write_data(MTPState *s)
 
     assert(d != NULL);
 
-    if (parent == NULL || !s->write_pending) {
-        usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
-                             0, 0, 0, 0);
+    switch (d->write_status) {
+    case WRITE_START:
+        if (!parent || !s->write_pending) {
+            usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, d->trans,
+                0, 0, 0, 0);
         return;
-    }
-
-    if (s->dataset.filename) {
-        path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
-        if (s->dataset.format == FMT_ASSOCIATION) {
-            d->fd = mkdir(path, mask);
-            goto free;
-        }
-        if ((s->dataset.size != 0xFFFFFFFF) && (s->dataset.size != d->offset)) {
-            usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
-                                 0, 0, 0, 0);
-            goto done;
-        }
-        d->fd = open(path, O_CREAT | O_WRONLY | O_CLOEXEC | O_NOFOLLOW, mask);
-        if (d->fd == -1) {
-            usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
-                                 0, 0, 0, 0);
-            goto done;
         }
 
-        /*
-         * Return success if initiator sent 0 sized data
-         */
-        if (!s->dataset.size) {
-            goto success;
-        }
+        if (s->dataset.filename) {
+            path = g_strdup_printf("%s/%s", parent->path, s->dataset.filename);
+            if (s->dataset.format == FMT_ASSOCIATION) {
+                d->fd = mkdir(path, mask);
+                goto free;
+            }
+            d->fd = open(path, O_CREAT | O_WRONLY |
+                         O_CLOEXEC | O_NOFOLLOW, mask);
+            if (d->fd == -1) {
+                usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
+                                     0, 0, 0, 0);
+                goto done;
+            }
 
-        rc = write_retry(d->fd, d->data, d->offset);
-        if (rc != d->offset) {
+            /* Return success if initiator sent 0 sized data */
+            if (!s->dataset.size) {
+                goto success;
+            }
+            if (d->length != MTP_WRITE_BUF_SZ && !d->pending) {
+                d->write_status = WRITE_END;
+            }
+        }
+        /* fall through */
+    case WRITE_CONTINUE:
+    case WRITE_END:
+        rc = write_retry(d->fd, d->data, d->data_offset,
+                         d->offset - d->data_offset);
+        if (rc != d->data_offset) {
             usb_mtp_queue_result(s, RES_STORE_FULL, d->trans,
                                  0, 0, 0, 0);
             goto done;
+        }
+        if (d->write_status != WRITE_END) {
+            return;
+        } else {
+            /* Only for < 4G file sizes */
+            if (s->dataset.size != 0xFFFFFFFF && d->offset != s->dataset.size) {
+                usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
+                                     0, 0, 0, 0);
+                goto done;
             }
-        /* Only for < 4G file sizes */
-        if (s->dataset.size != 0xFFFFFFFF && rc != s->dataset.size) {
-            usb_mtp_queue_result(s, RES_INCOMPLETE_TRANSFER, d->trans,
-                                 0, 0, 0, 0);
-            goto done;
+            usb_mtp_update_object(parent, s->dataset.filename);
         }
     }
 
@@ -1786,25 +1819,33 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         d->offset = 0;
         d->first = false;
         d->pending = false;
+        d->data_offset = 0;
+        d->write_status = WRITE_START;
     }
 
     if (d->pending) {
-        usb_mtp_realloc(d, MTP_WRITE_BUF_SZ);
-        d->length += MTP_WRITE_BUF_SZ;
+        memset(d->data, 0, d->length);
+        if (d->length != MTP_WRITE_BUF_SZ) {
+            usb_mtp_realloc(d, MTP_WRITE_BUF_SZ - d->length);
+            d->length += (MTP_WRITE_BUF_SZ - d->length);
+        }
         d->pending = false;
+        d->write_status = WRITE_CONTINUE;
+        d->data_offset = 0;
     }
 
-    if (d->length - d->offset > data_len) {
+    if (d->length - d->data_offset > data_len) {
         dlen = data_len;
     } else {
-        dlen = d->length - d->offset;
+        dlen = d->length - d->data_offset;
     }
 
     switch (d->code) {
     case CMD_SEND_OBJECT_INFO:
-        usb_packet_copy(p, d->data + d->offset, dlen);
+        usb_packet_copy(p, d->data + d->data_offset, dlen);
         d->offset += dlen;
-        if (d->offset == d->length) {
+        d->data_offset += dlen;
+        if (d->data_offset == d->length) {
             /* The operation might have already failed */
             if (!s->result) {
                 usb_mtp_write_metadata(s, dlen);
@@ -1815,19 +1856,26 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
         }
         break;
     case CMD_SEND_OBJECT:
-        usb_packet_copy(p, d->data + d->offset, dlen);
+        usb_packet_copy(p, d->data + d->data_offset, dlen);
         d->offset += dlen;
+        d->data_offset += dlen;
         if ((p->iov.size % 64) || !p->iov.size) {
             assert((s->dataset.size == 0xFFFFFFFF) ||
                    (s->dataset.size == d->offset));
 
+            if (d->length == MTP_WRITE_BUF_SZ) {
+                d->write_status = WRITE_END;
+            } else {
+                d->write_status = WRITE_START;
+            }
             usb_mtp_write_data(s);
             usb_mtp_data_free(s->data_out);
             s->data_out = NULL;
             return;
         }
-        if (d->offset == d->length) {
+        if (d->data_offset == d->length) {
             d->pending = true;
+            usb_mtp_write_data(s);
         }
         break;
     default:
-- 
2.19.2

[Qemu-devel] [PATCH v3 3/3] usb-mtp: replace the homebrew write with qemu_write_full

[Bandan Das]

qemu_write_full takes care of partial blocking writes,
as in cases of larger file sizes

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index b607c7be05..f4d9470493 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1630,24 +1630,16 @@ static char *utf16_to_str(uint8_t len, uint16_t *arr)
 /* Wrapper around write, returns 0 on failure */
 static uint64_t write_retry(int fd, void *buf, uint64_t size, off_t offset)
 {
-        uint64_t bytes_left = size, ret;
+        uint64_t ret = 0;
 
         if (lseek(fd, offset, SEEK_SET) < 0) {
             goto done;
         }
 
-        while (bytes_left > 0) {
-                ret = write(fd, buf, bytes_left);
-                if ((ret == -1) && (errno != EINTR || errno != EAGAIN ||
-                                    errno != EWOULDBLOCK)) {
-                        break;
-                }
-                bytes_left -= ret;
-                buf += ret;
-        }
+        ret = qemu_write_full(fd, buf, size);
 
 done:
-        return size - bytes_left;
+        return ret;
 }
 
 static void usb_mtp_update_object(MTPObject *parent, char *name)
-- 
2.19.2