Messages in this thread | | | Date | Wed, 13 Jul 2022 16:12:05 +0900 | From | Dominique Martinet <> | Subject | Re: [RFC PATCH] 9p: forbid use of mempool for TFLUSH |
| |
Kent Overstreet wrote on Wed, Jul 13, 2022 at 02:39:06AM -0400: > On 7/13/22 00:17, Dominique Martinet wrote: > > TFLUSH is called while the thread still holds memory for the > > request we're trying to flush, so mempool alloc can deadlock > > there. With p9_msg_buf_size() rework the flush allocation is > > small so just make it fail if allocation failed; all that does > > is potentially leak the request we're flushing until its reply > > finally does come.. or if it never does until umount. > > Why not just add separate mempools for flushes? We don't have to allocate > memory for big payloads so they won't cost much, and then the IO paths will > be fully mempool-ified :)
I don't think it really matters either way -- I'm much more worried about the two points I gave in the commit comment section: mempools not being shared leading to increased memory usage when many mostly-idle mounts (I know users who need that), and more importantly that the mempool waiting is uninterruptible/non-failible might be "nice" from the using mempool side but I'd really prefer users to be able to ^C out of a mount made on a bad server getting stuck in mempool_alloc at least.
It looked good before I realized all the ways this could hang, but now I just think for something like 9p it makes more sense to fail the allocation and the IO than to bounce forever trying to allocate memory we don't have.
Let's first see if you still see if you still see high order allocation failures when these are made much less likely with Chritisan's patch.
What I intend to push this cycle is in https://github.com/martinetd/linux/commits/9p-test up to 'net/9p: allocate appropriate reduced message buffers'; if you can easily produce them I'd appreciate if you could confirm if it helps.
(just waiting for Chritian's confirmation + adjusting the strcmp for rdma before I push it to 9p-next) -- Dominique
| |