GRUB has a bug where it waits a minimum of 400ms for every file it fetches over HTTP, unless the server serves it with Transfer-Encoding:chunked or the file just happens to be split into 20 TCP packets. When using pxeboot.img built with just pxe and http module (following instructions from https://www.gnu.org/software/grub/manual/html_node/Network.html), this causes an initial text menu to take about 7 seconds to load with all the modules being dynamically fetched. The SOB (statement of benefit?) of this patch is to fix this bug with the smallest change to existing data structures and logic. GRUB specifically checks for existing connections that are attached to file objects when seeking and closes them. New file requests don't have any sockets attached, and so it always opens a new connection to fetch new files. Adding the Connection:close header does not reduce performance.
The alternate patch on that bug ticket is a larger change, involving changing a (module-internal) data structure. I was also less sure of the flow of logic in http_receive(), so I did not immediately suggest it. It seems that the GRUB project is understandably conservative about changes, so I first provided the change that would have the smallest side-effect. I couched my phrasing with "I believe" and "I think" because I am not sure if I've fully understood the 3000+ lines of undocumented C code in net.c, netbuff.c, http.c, and tcp.c in only one week. I submitted this bug to bring a performance problem to your attention, and included a possible patch that fixes it for me, in the hope that someone on the project who is familiar with this code can review it and offer feedback. If the suggestion is to instead implement a more complete implementation of Connection:keepalive, I can try that out and offer a rough patch to improve that. As a newcomer to this project, I would feel more comfortable contributing a small change to fix the problem immediately instead of a large possibly-breaking change. Thank you! On Tue, Nov 15, 2016 at 12:44 PM, Daniel Kiper <dki...@net-space.pl> wrote: > On Tue, Nov 15, 2016 at 05:30:28PM +0300, Andrei Borzenkov wrote: > > On Tue, Nov 15, 2016 at 4:21 PM, Daniel Kiper <dki...@net-space.pl> > wrote: > > > On Sat, Nov 12, 2016 at 09:26:17AM -0800, Walter Huf wrote: > > >> GRUB's HTTP module declares support for HTTP/1.1, which defaults to > > >> Connection:keepalive. At the end of the content, the server holds the > TCP > > >> connection open waiting for the next request. > > >> It seems that grub_net_poll_cards() is watching for the HTTP module > to set > > > > > > Do you have a feeling or are you sure? I think that we have to be sure > here. > > > > > > > See http://savannah.gnu.org/bugs/?49531 which has more details. > > OK but I think that this should be a part of commit meesage. > > > >> net->stall, and otherwise waits the full 400ms GRUB_NET_INTERVAL to > return > > >> to processing. However, HTTP module only sets that flag in specific > > >> conditions: > > >> > > >> - parse_line detects that we are at the end of downloading a > chunked > > >> Transfer-Encoding > > >> - http_err detects a problem with the underlying TCP connection > > >> - http_receive has queued 20 netbuffer packets for processing > > >> > > >> If the file is small and takes less than 20 packets, and the server > is not > > >> using chunked encoding, grub_net_poll_cards() will wait the full 400ms > > >> before continuing to process and finish the file download. > > >> > > >> This patch sets Connection:close, which will tell the server to close > the > > >> connection as soon as it has finished sending the file. GRUB closes > any > > >> connections that are left open (in http_seek), so it does not change > > > > > > Why in http_seek? Is it correct or not? > > > > > >> performance. When the server disconnects, I think it triggers > http_err and > > > > > > "I think" is too soft. You have to be sure here. > > > > > >> then quits out of grub_net_poll_cards early. > > > > > > Lack of SOB. > > > > > > > I must have missed it. Is it now required? > > No (probably it should be sooner or later but we have to discuss this > issue separetly), however, nice to have. > > > > Have you tested this patch with large/huge number of files to > transfer? I have > > > a feeling that it can slowdown whole transfer in such cases due to > number of > > > connects/disconnects. Maybe this feature should be conditional thing. > > > > > > > Currently grub will always establish new connection in http_open -> > > http_establish, so it should not change anything from grub PoV, but > > reduce amount of lingering connections on server. But I would really > > like to explore another patch in above bug report - gracefully close > > connection when we finished receive data. That said, this one does not > > Looking (shortly) at problem description I am not sure that it will fix > the issue. Though I am not convinced that we should play with keep alive > too. Hence, better/nicer solution is appreciated. > > Daniel >
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
