Avoid calling 'stat' on HTTP GET #2300
Conversation
|
@amadio - I see the following unit test fails: Is that expected to succeed? I'm having a hard time figuring out how changes exclusively in |
|
Hi. I can reproduce an occasional segfault during XrdEcTests.IllegalVectorReadTest, e.g. using devel branch without this PR. So indeed it seems the crash is probably not related to this http change. (I'd have to confirm with Guilherme if he knows about any existing outstanding issue with the test). |
|
@bbockelm @smithdh I confirm that this test fails sporadically in the |
|
Hi! I'll be happy to review the change, which seems to make sense to me. I would appreciate a rebase though. Brian, could you please do it ? |
|
I managed to make the directory listing hang with a sufficiently large directory
Server just says: |
|
H. (Adding a comment as I'd said something in person during the workshop, of a possible reason for Fabrizio's observation of hang) - I had in mind an existing bad behaviour during header parsing (my last comment on #2072 ). However reading Fabrizio's description of the problem, that issue may not be relevant. |
|
@ffurano -- for recreating the failure you observe, what's the approximate size of directory for your directory listing? |
d645f04 to
17af84f
Compare
|
mine is 50k entries, and interestingly enough it enters a dodgy CPU-eating mode. However I think I have found the fix, it will likely come today after I finish testing. |
|
FWIW -- I created 100k files in a directory doing the following: and then invoked I didn't see any obvious failures. I did notice that the PROPFIND response buffers the entire 50MB response string in memory until all 100k entries were processed, then sent it at once. So, if you're doing something like a directory with 4M files in it, you need at least 20GB of memory for the response. (That's something that seems worth fixing but outside the purview of this particular PR.) @smithdh mentioned he has previously seen something like this so I'm summoning him on this ticket. |
|
The issue appears with GET, not PROPFIND |
|
The fix for this hang is in In general I like the idea behind this PR very much, and I like also the refactoring bits that it comes with. I'll test a bit further |
b48ac9d to
a914371
Compare
|
The latest force-push includes @ffurano's fix for directory listing. |
ffurano
left a comment
There was a problem hiding this comment.
To me the thing works and could provide a measurable benefit in some use cases.
Since the mod is based on a correct kXR_isDir behaviour, it may be subjected to the correct implementation of other Xrootd plugins. I would suggest someone else to test this with an xrootd in caching mode, whose behaviour and use cases I don't entirely master.
|
@osschar Would you mind giving this a go when you have time? |
|
@osschar - if you want an endpoint to test, this patch should be on all the OSDF cache servers (I believe Diego has CMS files present if you want CMS files to test). Alternatively, I can give you an RPM if you'd like. |
|
FWIW -- I've been exclusively testing and running this branch on all the OSDF caches (so, around 1PB a day of data delivery). I think it's in pretty good shape for that. |
|
Yes, the consensus on our side is that this is ready too. I just need to have the time to review it before merging. I will likely merge in the coming week. Thanks! |
amadio
left a comment
There was a problem hiding this comment.
@bbockelm Thanks for these changes, they look good. I'd like to request a few changes, however, in how the commits are structured. I think it makes sense to put first a commit adding sendFooterError and refactoring current code to use it where it makes sense, then add another commit refactoring the listing into the newly added PostProcessListing (including @ffurano's fix in the initial version, and with a Reviewed-by or Co-authored-by tag to acknowledge it). Then the commit refactoring the handling of GET requests to avoid calling stat. This will make it much easier to understand later. Thank you!
src/XrdHttp/XrdHttpReq.cc
Outdated
| if (!prot->Bridge->Run((char *) &xrdreq, (char *) res.c_str(), l)) { | ||
| prot->SendSimpleResp(404, NULL, NULL, (char *) "Could not run request.", 0, false); | ||
| return -1; | ||
| XrdHttpReq::mapXrdErrorToHttpStatus(); |
There was a problem hiding this comment.
You can use just mapXrdErrorToHttpStatus(); here, as we are inside XrdHttpReq anyway.
There was a problem hiding this comment.
Addressed in the latest commit (plus other, similar locations).
| { | ||
| // Ugly hack. Be careful with EOS! Test with vanilla XrdHTTP and EOS, separately | ||
| // A 404 on the preliminary stat() is fatal only | ||
| // in a manager. A non-manager will ignore the result and try anyway to open the file |
There was a problem hiding this comment.
@ccaffy I wonder if the changes introduced here will affect EOS, do you know? If that's possible, we should test this with EOS to make sure nothing there is relying on the previous behavior of stat + (open | dirlist).
There was a problem hiding this comment.
I would indeed appreciate a test from the EOS side as well!
|
Can we get this pull request resolved? It is now blocking work on pull request 2348 as any changes in that one rely on the changes in this pull request (i.e. returning the correct "AGE" response). It's not clear what all the holdups are, one certainly is that @bbockelm needs to respond to some change requests the other is a concern from @amadio, that said can we get some traction on this? |
The original GET implementation always started with a standalone `stat` call - whose results were only used to determine whether to invoke the directory listing code. This changes the GET to try to open the path by default and, if it is either a directory descriptor (or fails with EISDIR), then try doing the directory listing. By avoiding the standalone stat, we significantly reduce the number of filesystem calls done under high concurrency (as the OFS code will collapse all the open files into a single file handle).
Sending an error message is only possible if footer / trailers have been requested.
606f2d8 to
700fef3
Compare
Now that the footer code is in a common helper function, use that instead of duplicating it inline.
|
I'm ok with merging the current version of the code. However, in its current state I'd squash everything into a single commit. If you prefer to keep separate commits for this, please refactor as I suggested above, or let me know and I will squash and merge into |
|
I'm OK with squashing! (and it'll probably help with the timeline -- I'm hosting a workshop this week so my next technical context switch is potentially a week or two out) |
Instead of invoking a filesystem 'stat', then opening the path, change the GET code to always try the 'open' first. If the open fails with an kXR_isDirectory (i.e., `EISDIR`), then the code falls back to doing a directory listing. At a minimum, this will halve the number of 'stat' requests the underlying storage will see as 'stat' is done twice per request if the requests are sent sequentially. At best, it'll eliminate nearly all stat calls under high concurrency given the OFS will collapse the open files into a single handle and some OSS's will cache the results of the first fstat. Co-authored-by: Fabrizio Furano <fabrizio.furano@gmail.com>
Instead of invoking a filesystem 'stat', then opening the path, change the GET code to always try the 'open' first. If the open fails with an kXR_isDirectory (i.e., `EISDIR`), then the code falls back to doing a directory listing. At a minimum, this will halve the number of 'stat' requests the underlying storage will see as 'stat' is done twice per request if the requests are sent sequentially. At best, it'll eliminate nearly all stat calls under high concurrency given the OFS will collapse the open files into a single handle and some OSS's will cache the results of the first fstat. Co-authored-by: Fabrizio Furano <fabrizio.furano@gmail.com>
Instead of invoking a filesystem 'stat', then opening the path, change the GET code to always try the 'open' first. If the open fails with an kXR_isDirectory (i.e., `EISDIR`), then the code falls back to doing a directory listing. At a minimum, this will halve the number of 'stat' requests the underlying storage will see as 'stat' is done twice per request if the requests are sent sequentially. At best, it'll eliminate nearly all stat calls under high concurrency given the OFS will collapse the open files into a single handle and some OSS's will cache the results of the first fstat. Co-authored-by: Fabrizio Furano <fabrizio.furano@gmail.com>
Instead of invoking a filesystem 'stat', then opening the path, change the GET code to always try the 'open' first. If the open fails with an kXR_isDirectory (i.e., `EISDIR`), then the code falls back to doing a directory listing. At a minimum, this will halve the number of 'stat' requests the underlying storage will see as 'stat' is done twice per request if the requests are sent sequentially. At best, it'll eliminate nearly all stat calls under high concurrency given the OFS will collapse the open files into a single handle and some OSS's will cache the results of the first fstat. Co-authored-by: Fabrizio Furano <fabrizio.furano@gmail.com>
Instead of invoking a filesystem 'stat', then opening the path, change the GET code to always try the 'open' first. If the open fails with an kXR_isDirectory (i.e., `EISDIR`), then the code falls back to doing a directory listing. At a minimum, this will halve the number of 'stat' requests the underlying storage will see as 'stat' is done twice per request if the requests are sent sequentially. At best, it'll eliminate nearly all stat calls under high concurrency given the OFS will collapse the open files into a single handle and some OSS's will cache the results of the first fstat. Co-authored-by: Fabrizio Furano <fabrizio.furano@gmail.com>
Instead of invoking a filesystem 'stat', then opening the path, change the GET code to always try the 'open' first. If the open fails with an kXR_isDirectory (i.e., `EISDIR`), then the code falls back to doing a directory listing. At a minimum, this will halve the number of 'stat' requests the underlying storage will see as 'stat' is done twice per request if the requests are sent sequentially. At best, it'll eliminate nearly all stat calls under high concurrency given the OFS will collapse the open files into a single handle and some OSS's will cache the results of the first fstat. Co-authored-by: Fabrizio Furano <fabrizio.furano@gmail.com>
Instead of invoking a filesystem 'stat', then opening the path, change the GET code to always try the 'open' first. If the open fails with an kXR_isDirectory (i.e., `EISDIR`), then the code falls back to doing a directory listing. At a minimum, this will halve the number of 'stat' requests the underlying storage will see as 'stat' is done twice per request if the requests are sent sequentially. At best, it'll eliminate nearly all stat calls under high concurrency given the OFS will collapse the open files into a single handle and some OSS's will cache the results of the first fstat. Co-authored-by: Fabrizio Furano <fabrizio.furano@gmail.com>
This is a first attempt at "fixing" #2299. Instead of invoking a filesystem
stat, then opening the path, it changes the GET code to always try the 'open' first.If the open fails with an
kXR_isDirectory(i.e.,EISDIR), then the code falls back to doing a directory listing.At the minimum, this will halve the number of
statrequests the underlying storage will see asstatis done twice per request if the requests are sent sequentially. At best, it'll eliminate nearly allstats under high concurrency given the OFS will collapse the open files into a single handle and some OSS's will cache the results of the firstfstat.This state machine for GET is notoriously tricky so I'd appreciate a review by @ccaffy or @smithdh. Tests done by hand:
Rangerequest.Rangerequests.GETon a directory.