On Thu, Sep 28, 2023 at 10:08 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> On 9/28/23 12:46 AM, Amir Goldstein wrote:
> > I did not want to add an overlayfs specific workqueue for those
> > completions, because, as I'd mentioned before, I intend to move this
> > stacked file io infrastructure to common vfs code.
> >
> > I figured it's fine for overlayfs (or any stacked filesystem) to use its
> > own s_dio_done_wq for its own private needs.
> >
> > Please help me reassure that I got this right.
>
> Looks like you're creating it lazily as well, so probably fine to use
> the same wq rather than setup something new.
>
> > ret = -ENOMEM;
> > aio_req = kmem_cache_zalloc(ovl_aio_request_cachep, GFP_KERNEL);
> > if (!aio_req)
>
> Unrelated to this patch, but is this safe? You're allocating an aio_req
> from within the ->write_iter() handler, yet it's GFP_KERNEL? Seems like
> that should at least be GFP_NOFS, no?
I could be wrong, but since overlayfs does not have any page cache
of its own, I don't think memory reclaim poses a risk.
>
> That aside, punting to a workqueue is a very heavy handed solution to
> the problem. Maybe it's the only one you have, didn't look too closely
> at it, but it's definitely not going to increase your performance...
>
I bet it won't... but I need to worry about correctness.
What I would like to know, and that is something that I tried
to ask you in the Link: discussion, but perhaps I wasn't clear -
Are there any IOCB flags that the completion caller may set,
that will hint the submitter that completion is not from interrupt
context and that punting to workqueue is not needed?
The thing is that overlayfs does not submit io to blockdev -
It submits io to another underlying filesystem and the underlying
filesystem (e.g. ext4,xfs) is already likely to punt its write completion
to a workqueue (i.e. via iomap->end_io).
If I could tell when that is the case, then I could make punting to
workqueue in overlayfs conditional.
Anyway, I am not aware of any workloads that depend on high
io performance on overlayfs.
The only thing I have is Jiufei's commit message:
2406a307ac7d ("ovl: implement async IO routines")
who complained that overlayfs turned async io to sync io.
Jiufei,
Can you test this patch to see how it affects performance
in your workloads?
Thanks,
Amir.
