[#LU-16724] generic page pool

[LU-16724] generic page pool Created: 10/Apr/23 Updated: 12/Oct/23
Status:	Open
Project:	Lustre
Component/s:	None
Affects Version/s:	None
Fix Version/s:	None

Type:

Improvement

Priority:

Minor

Reporter:

Patrick Farrell

Assignee:

Patrick Farrell

Resolution:

Unresolved

Votes:

Labels:

None

Issue Links:

Related

Rank (Obsolete):

9223372036854775807

Description

Looking at the unaligned DIO work in LU-13805, Andreas pointed out we should consider how we're getting pages for our unaligned DIO, the already existing client side encryption, and our future compression feature (~~LU-10092~~).

All three features need to allocate individual pages of memory for what are essentially bounce buffers. The encryption feature uses a page pool, which seems like a good idea for the others.

This ticket is to try to settle on one way to do this and ideally have only a single page pool (if that's our solution).

sebastien , can you talk about why the page pool was chosen? Was it performance or functionality?

I assume page pool performance is similar to or better than getting pages from the kernel allocator. My only concern is wasting memory by having too large of a pool re-allocated pages. In theory, we could need several GiB of bounce buffers if there is several GiB of outstanding IO.

However, it looks like there are shrinkers for the page pools, so if the pages are unused, the pool can be responsive to memory pressure.

So my first guess at a solution is this:
Have a single shared page pool ("lustre bounce buffers" or similar), with some fairly small pre-allocated size, then it can grow and shrink as needed. Because page pools respond to kernel memory pressure, we shouldn't have the problem of holding on to unneeded memory.

So, we have one page pool for all our bounce buffers, and we don't worry too much about starting size except to keep it small, since the required size will be highly variable. If a customer is not using any of these features, the total need is zero (so we should keep the cost small in this case). If they are doing heavy IO while using one or more of these features, they could easily require several GiB at once. So we pay the cost of allocating the necessary memory the first time it's used, then we don't have to pay that cost again unless the kernel asks us to shrink it.

How does this sound? sebastien , adilger , ablagodarenko

Comments

Comment by Artem Blagodarenko [ 10/Apr/23 ]

Hi paf0186 , I am changing sec_bulk right now to address client-size compression needs. I am about to finish and going to share the code soon. Can we get this patch as a base for the solution? Thanks.

Comment by Patrick Farrell [ 10/Apr/23 ]

Sure, Artem.

Just to make sure I understand:
You think a single shared page pool like this is a good idea and you'd like a patch so you can base current work on it? (I am happy to do this patch) Or do you want to use your patch as a base for this work?

Comment by Artem Blagodarenko [ 10/Apr/23 ]

paf0186 Actually I mean that my patch adds some kind of generalization to the current encryption pool, so probably we can use it as a base for the next optimizations. But if we decided to rewrite the code completely (for any important reason) I am ready to use this code ofcause.

Comment by Patrick Farrell [ 10/Apr/23 ]

OK, that makes sense - I'm glad I asked. I am happy to wait for your patch, it will be easy to integrate a page pool in to the unaligned DIO code. It just needs to get pages from somewhere, it doesn't really have any special requirements.

Comment by Patrick Farrell [ 10/Apr/23 ]

So, this is a bit of a separate question, but I wanted to answer it here because it's related and I think it's reasonable to ask:

I do not think we can, realistically, avoid extra buffering for the case where we're doing unaligned DIO and compression or encryption. Compression and encryption both operate on the data at lower layers (basically OSC/ptlrpc), and they expect aligned memory, as does our RDMA (in general). So something has to ensure the memory is aligned before it gets to these layers. For buffered I/O, that's done by copying in to the page cache, for regular direct I/O, it is required in userspace. For unaligned direct I/O, we have to do the aligning in the DIO code, which requires copying in to (/out of) an aligned buffer. So we do end up with three copies in that case - userspace, a copy in the DIO layer, and one for compression (or encryption).

Avoiding that would require teaching the encryption and compression code to handle copying to/from unaligned user memory, and this means splitting the copying work up differently in a different spot (we currently split it by stripe). This is possible, but it don't think it would be fun to code it up and I can see some big stumbling blocks around how we handle the iovec from VFS, etc, since we'd have many threads hitting it at once.

So I think this is worth considering if we end up with IO to compressed files as a major performance concern (for example, if they're often used for 'hot' or hot-ish data, rather than colder data), but I think it should be left for later rather than trying to work it in to the initial versions of any of this.

Comment by Andreas Dilger [ 10/Apr/23 ]

Patrick, my understanding is that the page pools were added for encryption for performance reasons, to reduce the overhead of repeated allocation/free of the bounce buffers. Sebastien discussed this in his LUG2021 Lustre Client Encryption presentation.

The compression page pool has some stronger requirements vs. the encryption or DIO page pool, because it needs multi-page allocations (either higher-order alloc_page() or vmalloc()) in order to provide the compression code with sufficient contiguous address space for the compression code to run (it unfortunately cannot currently work with page-fragmented source and/or target buffers). That should be a superset of the requirements for either of the other two uses.

Doing the unaligned DIO->buffer copying in the same step as compression/encryption would potentially be an optimization worthwhile to do if those use combined use cases are common, but it isn't clear whether they overlap a lot? The unaligned DIO will be useful for poorly-formed IO sized and aligned writes, which doesn't work well for either client-side compression or encryption since they both need to be page sized (or a multiple thereof for compression) and aligned.

Comment by Gerrit Updater [ 11/Sep/23 ]

"Patrick Farrell <pfarrell@whamcloud.com>" uploaded a new patch: https://review.whamcloud.com/c/fs/lustre-release/+/52335
Subject: LU-16724 ptlrc: ptlrpc: extend sec bulk functionality
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 991aeaad4f43b7eec3bb33f7bc420aa5b92bf7ea

Comment by Patrick Farrell [ 11/Oct/23 ]

I'm also going to stick some cleanup patches under here.