Re: [PATCH] Rewrite and fix grub_bufio_read()

[Andrei Borzenkov]

28.04.2016 14:11, Stefan Fritsch пишет:
> We had problems downloading large (10-100 MB) gziped files via http
> with grub. After some debugging, I think I have found the reason in
> grub_bufio_read, which leads to wrong data being passed on. This patch
> fixes the issues for me.  I did my tests with a somewhat older
> snapshot, but the bufio.c file is the same.
> 

Please send minimal patch that fixes bug in bufio so we can actually see
where the bug is and apply bug fix for 2.02. It is impossible to deduce
from your complete rewrite and this patch is too intrusive for 2.02
irrespectively of considerations below.

> Cheers,
> Stefan
> 
> The grub_bufio_read() had some issues:
> 
> * in the calculation of next_buf, it assumed that bufio->block_size is a
>   power of 2, which is not always the case (see code in grub_bufio_open()).
> 

All current callers set it to power of 2, although you are right that it
should verify it.

> * it assumed that the len passed to grub_bufio_read() is not larger
>   than bufio->block_size. I have no idea if this is always true, but it
>   seems rather fragile to depend on that.
> 

Where do you got this impression from? It reads arbitrary size, there is
no assumption about len. Otherwise netboot had not worked at all. The
logic is - rest of previous buffer, direct read until farthest buffer
boundary, partial buffer read at the end. Low level file is responsible
for the second step, so there is no loop needed.

> * depending on the seek patterns, it can do much unbuffered reading
> 

Yes, if you read large chunk as in reading kernel or initrd it simply
does direct read into provided buffer, thus avoiding extra memory copy.

> This patch rewrites the function, making it more readable and fixing
> these issues. It also loops until the requested amount of bytes has
> been read.

Well, your patch now adds extra copying of every block thus making it
less efficient for large reads which are the primary objective. So I am
afraid we cannot use this patch; but it would be great if you could
provide analysis of problem you observed in current code, showing where
and why it happens, so we could fix it.

> ---
>  grub-core/io/bufio.c | 104 
> ++++++++++++++++++++-------------------------------
>  1 file changed, 40 insertions(+), 64 deletions(-)
> 
> diff --git a/grub-core/io/bufio.c b/grub-core/io/bufio.c
> index 2243827..2ee96e4 100644
> --- a/grub-core/io/bufio.c
> +++ b/grub-core/io/bufio.c
> @@ -103,81 +103,57 @@ static grub_ssize_t
>  grub_bufio_read (grub_file_t file, char *buf, grub_size_t len)
>  {
>    grub_size_t res = 0;
> -  grub_off_t next_buf;
>    grub_bufio_t bufio = file->data;
>    grub_ssize_t really_read;
>  
> -  if (file->size == GRUB_FILE_SIZE_UNKNOWN)
> -    file->size = bufio->file->size;
> -
> -  /* First part: use whatever we already have in the buffer.  */
> -  if ((file->offset >= bufio->buffer_at) &&
> -      (file->offset < bufio->buffer_at + bufio->buffer_len))
> +  while (len > 0)
>      {
> -      grub_size_t n;
> -      grub_uint64_t pos;
> -
> -      pos = file->offset - bufio->buffer_at;
> -      n = bufio->buffer_len - pos;
> -      if (n > len)
> -        n = len;
> -
> -      grub_memcpy (buf, &bufio->buffer[pos], n);
> -      len -= n;
> -      res += n;
> -
> -      buf += n;
> -    }
> -  if (len == 0)
> -    return res;
>  
> -  /* Need to read some more.  */
> -  next_buf = (file->offset + res + len - 1) & ~((grub_off_t) 
> bufio->block_size - 1);
> -  /* Now read between file->offset + res and bufio->buffer_at.  */
> -  if (file->offset + res < next_buf)
> -    {
> -      grub_size_t read_now;
> -      read_now = next_buf - (file->offset + res);
> -      grub_file_seek (bufio->file, file->offset + res);
> -      really_read = grub_file_read (bufio->file, buf, read_now);
> -      if (really_read < 0)
> -     return -1;
>        if (file->size == GRUB_FILE_SIZE_UNKNOWN)
>       file->size = bufio->file->size;
> -      len -= really_read;
> -      buf += really_read;
> -      res += really_read;
>  
> -      /* Partial read. File ended unexpectedly. Save the last chunk in 
> buffer.
> -       */
> -      if (really_read != (grub_ssize_t) read_now)
> +      /* First part: use whatever we already have in the buffer.  */
> +      if ((file->offset + res >= bufio->buffer_at) &&
> +       (file->offset + res < bufio->buffer_at + bufio->buffer_len))
>       {
> -       bufio->buffer_len = really_read;
> -       if (bufio->buffer_len > bufio->block_size)
> -         bufio->buffer_len = bufio->block_size;
> -       bufio->buffer_at = file->offset + res - bufio->buffer_len;
> -       grub_memcpy (&bufio->buffer[0], buf - bufio->buffer_len,
> -                    bufio->buffer_len);
> -       return res;
> +       grub_size_t n;
> +       grub_uint64_t pos;
> +
> +       pos = file->offset + res - bufio->buffer_at;
> +       n = bufio->buffer_len - pos;
> +       if (n > len)
> +         n = len;
> +
> +       grub_memcpy (buf, &bufio->buffer[pos], n);
> +       len -= n;
> +       res += n;
> +
> +       buf += n;
>       }
> -    }
> +      if (len == 0)
> +     return res;
> +
> +      /* Need to read some more.  */
> +
> +      if ((file->offset + res < bufio->buffer_at) ||
> +       (file->offset + res >= bufio->buffer_at + bufio->block_size))
> +        {
> +       /* There is no space left in the buffer, move start of buffer */
> +       bufio->buffer_at = file->offset + res;
> +       bufio->buffer_len = 0;
> +        }
> +
> +      grub_file_seek (bufio->file, bufio->buffer_at + bufio->buffer_len);
>  
> -  /* Read into buffer.  */
> -  grub_file_seek (bufio->file, next_buf);
> -  really_read = grub_file_read (bufio->file, bufio->buffer,
> -                             bufio->block_size);
> -  if (really_read < 0)
> -    return -1;
> -  bufio->buffer_at = next_buf;
> -  bufio->buffer_len = really_read;
> -
> -  if (file->size == GRUB_FILE_SIZE_UNKNOWN)
> -    file->size = bufio->file->size;
> -
> -  if (len > bufio->buffer_len)
> -    len = bufio->buffer_len;
> -  grub_memcpy (buf, &bufio->buffer[file->offset + res - next_buf], len);
> -  res += len;
> +      really_read = grub_file_read (bufio->file, 
> &bufio->buffer[bufio->buffer_len],
> +                                 bufio->block_size - bufio->buffer_len);
> +
> +      if (really_read < 0)
> +     return -1;
> +      if (really_read == 0)
> +     return res;
> +      bufio->buffer_len += really_read;
> +    }
>  
>    return res;
>  }
>