Wei - a side note, when reviewing huuuuge patches like this _please_ remove irrelevant lines, it's really hard for me to respond and easy for me to miss stuff scrolling through >600 lines. Thanks! Also - you're reviewing an out of date series, the new version has changed a lot, see [0]. [0]:https://lore.kernel.org/all/20241108135708.48567-1-lorenzo.stoakes@xxxxxxxxxx/ On Mon, Nov 11, 2024 at 07:07:19AM +0000, Wei Yang wrote: > On Thu, Nov 07, 2024 at 07:01:37PM +0000, Lorenzo Stoakes wrote: > >+If you want to **write** VMA metadata fields, then things vary depending on the > >+field (we explore each VMA field in detail below). For the majority you must: > >+ > >+* Obtain an mmap write lock at the MM granularity via :c:func:`!mmap_write_lock` (or a > >+ suitable variant), unlocking it with a matching :c:func:`!mmap_write_unlock` when > >+ you're done with the VMA, *and* > >+* Obtain a VMA write lock via :c:func:`!vma_start_write` for each VMA you wish to > >+ modify, which will be released automatically when :c:func:`!mmap_write_unlock` is > >+ called. > >+* If you want to be able to write to **any** field, you must also hide the VMA > >+ from the reverse mapping by obtaining an **rmap write lock**. > > Here I am a little confused. > > I guess it wants to say mmap write lock and VMA write lock should be obtained, > while rmap write lock is optional for write operation. And this is what > illustrated in below section "VMA fields". > > But here says 'write to "any" field'. This seems not what it shows below. > > Or maybe I misunderstand this part. It opens with 'for the majority' as in, for the _majority_ you need the mmap write, VMA write lock. To able to adjust _all_ you need the rmap write lock too. I have to trade off with 'summarising' how you get a write lock vs. the specifics, I mean I literally give a table immediately after this explicitly listing what is required. If I gave just that, it'd be confusing and people wouldn't maybe realise that _mostly_ mmap write, VMA write is enough. If I don't mention the rmap thing then people might be confused as to why. Given I give the correct requirements in the table I don't tnink this is a problem. [snip] > >+.. table:: Virtual layout fields > >+ > >+ ===================== ======================================== =========== > >+ Field Description Write lock > >+ ===================== ======================================== =========== > >+ :c:member:`!vm_start` Inclusive start virtual address of range mmap write, > >+ VMA describes. VMA write, > >+ rmap write. > >+ :c:member:`!vm_end` Exclusive end virtual address of range mmap write, > >+ VMA describes. VMA write, > >+ rmap write. > >+ :c:member:`!vm_pgoff` Describes the page offset into the file, rmap write. > > ^ > I am afraid this one is a typo? This is fixed in v2. [snip]
