On 26/05/26 04:07PM, Dave Jiang wrote: > > > On 5/22/26 12:18 PM, John Groves wrote: > > From: John Groves <[email protected]> > > > > The comment in dax_folio_reset_order() claims that DAX maintains an > > invariant where folio->share != 0 only when folio->mapping == NULL, > > implying folio->share is zero whenever mapping is non-NULL. This is > > misleading because folio->share and folio->index are a union -- for > > non-shared folios with mapping != NULL, reading folio->share returns > > the file page offset (folio->index), which is typically non-zero. > > > > Reword the comment to accurately describe the union aliasing: the > > assignment clears whichever interpretation of the union word is active > > (index for non-shared folios, share for shared folios), which is correct > > because the folio is being released in either case. > > > > No functional change -- the code was already correct, only the > > justification was wrong. > > > > Fixes: 59eb73b98ae0b ("dax: Factor out dax_folio_reset_order() helper") > > > > Reviewed-by: Jonathan Cameron <[email protected]> > > Signed-off-by: John Groves <[email protected]> > > --- > > fs/dax.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/fs/dax.c b/fs/dax.c > > index 6d175cd47a99b..df19c9317d10e 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -392,12 +392,12 @@ int dax_folio_reset_order(struct folio *folio) > > int order = folio_order(folio); > > > > /* > > - * DAX maintains the invariant that folio->share != 0 only when > > - * folio->mapping == NULL (enforced by dax_folio_make_shared()). > > - * Equivalently: folio->mapping != NULL implies folio->share == 0. > > - * Callers ensure share has been decremented to zero before > > - * calling here, so unconditionally clearing both fields is > > - * correct. > > + * Clear the mapping and the index/share union word. folio->share > > + * and folio->index occupy the same union in struct folio. For > > + * non-shared folios (mapping != NULL), the union holds folio->index > > + * (file page offset); for shared folios (mapping == NULL), it holds > > + * folio->share (reference count). Either way, we are releasing the > > + * folio and both fields should be zeroed. > > In the old comments, there is the pre-condition that "callers ensure share > has been decremented to zero before calling here." Is this precondition > remain true? Maybe should leave that comment in if that is the case?
That precondition is no longer universally true. fsdev_clear_folio_state() calls this at probe time on folios that may be in arbitrary state. The new wording justifies the zeroing correctly for all callers: we are releasing the folio so both the mapping and the union word (whether it holds an index or a share count) must be cleared. So I think this is right... Thanks for the review Dave! John <snip>
