On 3/14/23 3:26?AM, Christian Brauner wrote:
> On Tue, Mar 07, 2023 at 08:10:32PM -0700, Jens Axboe wrote:
>> @@ -493,9 +507,13 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
>> int copied;
>>
>> if (!page) {
>> - page = alloc_page(GFP_HIGHUSER | __GFP_ACCOUNT);
>> + gfp_t gfp = __GFP_HIGHMEM | __GFP_ACCOUNT;
>> +
>> + if (!nonblock)
>> + gfp |= GFP_USER;
>
> Just for my education: Does this encode the assumpation that the
> non-blocking code can only be reached from io_uring and thus GFP_USER
> can be dropped for that case? IOW, if there's other code that could in
> the future reach the non blocking condition would this still be correct?
You can already reach that if you do preadv2(..., RWF_NOWAIT). There
should be no assumptions here on the user of it, semantics should be the
same. The gfp mask is just split so we avoid __GFP_WAIT for the
nonblocking case.
>
>> + page = alloc_page(gfp);
>> if (unlikely(!page)) {
>> - ret = ret ? : -ENOMEM;
>> + ret = ret ? : nonblock ? -EAGAIN : -ENOMEM;
>
> Hm, could we try and avoid the nested "?:?:" please. Imho, that's easy
> to misparse. Idk, doesn't need to be exactly that code but sm like:
>
> if (!nonblock) {
> gfp |= GFP_USER;
> ret = -EAGAIN;
> } else {
> ret = -ENOMEM;
> }
>
> page = alloc_page(gfp);
> if (unlikely(!page))
> break;
> else
> ret = 0;
> pipe->tmp_page = page;
>
> or sm else.
Yeah this is much better, I think I was a bit too lazy here, not really
a fan of ternaries myself... I'll fix that up. Thanks!
--
Jens Axboe
