Re: [PATCH 06/16] huge tmpfs: shmem_is_huge(vma, inode, index)

On Thu, Aug 5, 2021 at 10:21 PM Hugh Dickins [off-list ref] wrote:

On Wed, 4 Aug 2021, Yang Shi wrote:

quoted

On Wed, Aug 4, 2021 at 1:28 AM Hugh Dickins [off-list ref] wrote:

quoted

Thanks, but despite us agreeing that the race is too unlikely to be worth
optimizing against, it does still nag at me ever since you questioned it:
silly, but I can't quite be convinced by my own dismissals.

I do still want to get rid of SGP_HUGE and SGP_NOHUGE, clearing up those
huge allocation decisions remains the intention; but now think to add
SGP_NOALLOC for collapse_file() in place of SGP_NOHUGE or SGP_CACHE -
to rule out that possibility of mischarge after racing hole-punch,
no matter whether it's huge or small.  If any such race occurs,
collapse_file() should just give up.

This being the "Stupid me" SGP_READ idea, except that of course would
not work: because half the point of that block in collapse_file() is
to initialize the !Uptodate pages, whereas SGP_READ avoids doing so.

There is, of course, the danger that in fixing this unlikely mischarge,
I've got the code wrong and am introducing a bug: here's what a 17/16
would look like, though it will be better inserted early.  I got sick
of all the "if (page "s, and was glad of the opportunity to fix that
outdated "bring it back from swap" comment - swap got done above.

What do you think? Should I add this in or leave it out?

Thanks for keeping investigating this. The patch looks good to me. I
think we could go this way. Just a nit below.

Thanks, I'll add it into the series, a patch before SGP_NOHUGE goes away;
but I'm not intending to respin the series until there's more feedback
from others - fcntl versus fadvise is the main issue so far.

Thanks, yeah, no hurry to repost.

quoted

quoted

--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h

@@ -108,6 +108,7 @@ extern unsigned long shmem_partial_swap_usage(struct address_space *mapping,
 /* Flag allocation requirements to shmem_getpage */
 enum sgp_type {
        SGP_READ,       /* don't exceed i_size, don't allocate page */
+       SGP_NOALLOC,    /* like SGP_READ, but do use fallocated page */

The comment looks misleading, it seems SGP_NOALLOC does clear the
Uptodate flag but SGP_READ doesn't. Or it is fine not to distinguish
this difference?

I think you meant to say, SGP_NOALLOC does *set* the Uptodate flag but
SGP_READ doesn't.  And a more significant difference, as coded to suit
collapse_file(), is that SGP_NOALLOC returns failure on hole, whereas
SGP_READ returns success: I should have mentioned that.

Yes, I mean "set". Sorry for the confusion.

When I wrote "like SGP_READ" there, I just meant "like what's said in
the line above": would "ditto" be okay with you, and I say
        SGP_NOALLOC,    /* ditto, but fail on hole, or use fallocated page */

I don't really want to get into the "Uptodate" business there.
And I'm afraid someone is going to ask me to write multi-line comments
on each of those SGP_flags, and I'm going to plead "read the source"!

OK, I'm fine as is.

Oh, now I see why you said SGP_NOALLOC does clear the Uptodate flag:
"goto clear", haha: when we clear the page we set the Uptodate flag.

And I may have another patch to slot in: I was half expecting you to
question why SGP_READ behaves as it does, so in preparing its defence
I checked, and found it was not doing quite what I remembered: changes
were made a long time ago, which have left it slightly suboptimal.
But that really has nothing to do with the rest of this series,
and I don't need to run it past you before reposting.

I hope that some of the features in this series can be useful to you.

Thanks, I will see.

Thanks,
Hugh