From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1avq4j-00075a-LV for mharc-grub-devel@gnu.org; Thu, 28 Apr 2016 13:50:05 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52438) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avq4d-0006yW-FL for grub-devel@gnu.org; Thu, 28 Apr 2016 13:50:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avq4Z-0004d4-8j for grub-devel@gnu.org; Thu, 28 Apr 2016 13:49:59 -0400 Received: from mail-lf0-x232.google.com ([2a00:1450:4010:c07::232]:33298) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avq4Y-0004c8-Rh for grub-devel@gnu.org; Thu, 28 Apr 2016 13:49:55 -0400 Received: by mail-lf0-x232.google.com with SMTP id y84so92529086lfc.0 for ; Thu, 28 Apr 2016 10:49:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-transfer-encoding; bh=d4wMi4QgCbdSpnaxtV08CQbIOym63bWiWD8rJX9RAxY=; b=y6QUK/1gne62qsAgycf6vgOsMITXDYLf8j+cMan+AK2G6jYNfjGFbQ/kLJLU56HYc7 9LbjZIidJ4ZuGqjOZiPSKLZfjYIN7SIj6/XhxewpVgMCw9Nx01VSq5spH28NFizNm1z6 ZCOUHMov0jopUdjVCTuts5plX/7K1nb/YX9mnLHPmivRHbT9vhA//6FjWd/YuCLclKt2 Ox1VyDSh0vf5WlfO3PeA5tO9l53JJ+MorPxYjnxAK/mxTsE6wmw6tUzifxy0u9pWqd0a Vm3Nqkvj3gfJq5T/tKFHaZWAGMWbAaYx/zc8Z7hBoInfrrQYkJFmQVo63NNXGnv7azia 9nvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=d4wMi4QgCbdSpnaxtV08CQbIOym63bWiWD8rJX9RAxY=; b=FtWeqTx7ymYl6PC8wCRMxPkwJQplxArlMk/p6HoNTGRJHlFT4yHAEmmsFWrbSXM/Lz VQLEEqm7jkjCkV/sB6hMHO4Qbsr3ERZiiWODdz6UsRbbz7L7XLywzBRdoktNRmqENnQm 9aVqoxEWj8R0CVikD7pN4AxcxrFzKnVbSQ5NVV4aG2J9HVjBn3jG6u+ZmTtGR1DblpU6 kQX9t6w9nxjSNCxmgaDV/IZIU16Bse92l7rJLu0O6iU1O4pKUB75J2nd8DMsSKNRH+cC +j1h43o3Uw5wLI8hU1Oj4ZitgEmFyW66xAxi3owSKWdWOlafEs3hKhavWt4S/cd0yBTr Ze2w== X-Gm-Message-State: AOPr4FWVxZ5w20NFKdqBJkFsXnHc29tFuxAUrecT1RWOoSNgfMkPb0pjDy49bDIo0qhJCQ== X-Received: by 10.112.140.3 with SMTP id rc3mr6751609lbb.85.1461865793699; Thu, 28 Apr 2016 10:49:53 -0700 (PDT) Received: from [192.168.1.42] (ppp109-252-90-74.pppoe.spdop.ru. [109.252.90.74]) by smtp.gmail.com with ESMTPSA id ez1sm1805828lbc.29.2016.04.28.10.49.51 for (version=TLSv1/SSLv3 cipher=OTHER); Thu, 28 Apr 2016 10:49:52 -0700 (PDT) Subject: Re: [PATCH] Rewrite and fix grub_bufio_read() To: The development of GNU GRUB References: From: Andrei Borzenkov X-Enigmail-Draft-Status: N1110 Message-ID: <57224D3F.1080704@gmail.com> Date: Thu, 28 Apr 2016 20:49:51 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:4010:c07::232 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Apr 2016 17:50:03 -0000 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; > } >