On Tue, Oct 1, 2024 at 6:33 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
>
> Nit: extra blank line.
>
> > static inline void free_swap_and_cache(swp_entry_t entry)
> > {
> > free_swap_and_cache_nr(entry, 1);
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 0613421e09e7..e3f72f99be32 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1561,7 +1561,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> > __GFP_HIGH | __GFP_NOMEMALLOC | __GFP_NOWARN,
> > NULL) == 0) {
> > shmem_recalc_inode(inode, 0, nr_pages);
> > - swap_shmem_alloc(swap, nr_pages);
> > + swap_duplicate_nr(swap, nr_pages);
> > shmem_delete_from_page_cache(folio, swp_to_radix_entry(swap));
> >
> > mutex_unlock(&shmem_swaplist_mutex);
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 0cded32414a1..9bb94e618914 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -1381,12 +1381,6 @@ static unsigned char __swap_entry_free_locked(struct swap_info_struct *si,
> > if (usage == SWAP_HAS_CACHE) {
> > VM_BUG_ON(!has_cache);
> > has_cache = 0;
> > - } else if (count == SWAP_MAP_SHMEM) {
> > - /*
> > - * Or we could insist on shmem.c using a special
> > - * swap_shmem_free() and free_shmem_swap_and_cache()...
> > - */
> > - count = 0;
> > } else if ((count & ~COUNT_CONTINUED) <= SWAP_MAP_MAX) {
> > if (count == COUNT_CONTINUED) {
> > if (swap_count_continued(si, offset, count))
> > @@ -3626,7 +3620,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> >
> > offset = swp_offset(entry);
> > VM_WARN_ON(nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER);
> > - VM_WARN_ON(usage == 1 && nr > 1);
> > ci = lock_cluster_or_swap_info(si, offset);
> >
> > err = 0;
> > @@ -3652,6 +3645,13 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> > err = -EEXIST;
> > } else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) {
> > err = -EINVAL;
> > + } else {
> > + /*
> > + * The only swap_duplicate_nr() caller that passes nr > 1 is shmem,
> > + * who never re-duplicates any swap entry it owns. So this should
>
> nit: I think "which" is the right word here, but I am not a native speaker :)
Yeah I think it should be which. Fix(let) incoming.
>
> > + * not happen.
> > + */
> > + VM_WARN_ON(nr > 1 && (count & ~COUNT_CONTINUED) == SWAP_MAP_MAX);
>
> Why not return an error in this case? I think we should add recovery
> for bugs when it's possible and simple, which I believe is the case
> here.
>
> In shmem_writepage() we can add a WARN if swap_duplicate_nr() fails,
> or propagate an error to the caller as well (perhaps this belongs in a
> separate patch that does this for swap_shmem_alloc() first).
>
> Sorry if I am being paranoid here, please let me know if this is the case.
I was debating between WARN-ing here, and returning -ENOMEM and
WARN-ing at shmem's callsite.
My thinking is that if we return -ENOMEM here, it will work in the
current setup, for both shmem and other callsites. However, in the
future, if we add another user of swap_duplicate_nr(), this time
without guaranteeing that we won't need continuation, I think it won't
work unless we have the fallback logic in place as well:
while (!err && __swap_duplicate(entry, 1, nr) == -ENOMEM)
err = add_swap_count_continuation(entry, GFP_ATOMIC);
>
> > }
> >
> > if (err)
> > @@ -3686,27 +3686,28 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> > return err;
> > }
> >
> > -/*
> > - * Help swapoff by noting that swap entry belongs to shmem/tmpfs
> > - * (in which case its reference count is never incremented).
> > - */
> > -void swap_shmem_alloc(swp_entry_t entry, int nr)
> > -{
> > - __swap_duplicate(entry, SWAP_MAP_SHMEM, nr);
> > -}
> > -
> > -/*
> > - * Increase reference count of swap entry by 1.
> > +/**
> > + * swap_duplicate_nr() - Increase reference count of nr contiguous swap entries
> > + * by 1.
>
> Can we avoid the line break by using "refcount" instead of "reference count"?
>
> > + *
> > + * @entry: first swap entry from which we want to increase the refcount.
> > + * @nr: Number of entries in range.
> > + *
> > * Returns 0 for success, or -ENOMEM if a swap_count_continuation is required
> > * but could not be atomically allocated. Returns 0, just as if it succeeded,
> > * if __swap_duplicate() fails for another reason (-EINVAL or -ENOENT), which
> > * might occur if a page table entry has got corrupted.
> > + *
> > + * Note that we are currently not handling the case where nr > 1 and we need to
> > + * add swap count continuation. This is OK, because no such user exists - shmem
> > + * is the only user that can pass nr > 1, and it never re-duplicates any swap
> > + * entry it owns.
>
> Do we need this comment when we have the WARN + comment in __swap_duplicate()?
Here I'm just being cautious and include the limitation of the
function in the API documentation itself.
No strong opinions though.
>
> > */
> > -int swap_duplicate(swp_entry_t entry)
> > +int swap_duplicate_nr(swp_entry_t entry, int nr)
> > {
> > int err = 0;
> >
> > - while (!err && __swap_duplicate(entry, 1, 1) == -ENOMEM)
> > + while (!err && __swap_duplicate(entry, 1, nr) == -ENOMEM)
> > err = add_swap_count_continuation(entry, GFP_ATOMIC);
> > return err;
> > }
> > --
> > 2.43.5
