Re: [PATCH V6 2/2] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem

[Suren Baghdasaryan]

On Wed, Feb 8, 2023 at 6:55 AM Charan Teja Kalla
<quic_charante@xxxxxxxxxxx> wrote:
>
> Thanks Suren!!
>
> On 2/8/2023 4:18 AM, Suren Baghdasaryan wrote:
> >> +static int shmem_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
> >> +{
> >> +       loff_t endbyte;
> >> +       pgoff_t start_index;
> >> +       pgoff_t end_index;
> >> +       struct address_space *mapping;
> >> +       struct inode *inode = file_inode(file);
> >> +       int ret = 0;
> >> +
> >> +       if (S_ISFIFO(inode->i_mode))
> >> +               return -ESPIPE;
> >> +
> >> +       mapping = file->f_mapping;
> >> +       if (!mapping || len < 0 || !shmem_mapping(mapping))
> >> +               return -EINVAL;
> >> +
> >> +       endbyte = fadvise_calc_endbyte(offset, len);
> >> +
> >> +       start_index = offset >> PAGE_SHIFT;
> >> +       end_index   = endbyte >> PAGE_SHIFT;
> >> +       switch (advice) {
> >> +       case POSIX_FADV_DONTNEED:
> > Should (SHMEM_I(inode)->flags & VM_LOCKED) be checked here too?
> >
> Is this w.r.t context from shmem_lock() perspective which does set this
> flag?  If so, Isn't the PageUnevictable check cover this part? And to
> avoid unnecessary Unevictable check later on the locked shmem file, How
> about just checking mapping_unevictable() before performing
> shmem_fadvise_dontneed)()? Please let me know If I failed to get your point.

Yes, you got my point and checking for mapping_unevictable() should
work fine I think.

>
> >> +               ret = shmem_fadvise_dontneed(mapping, start_index, end_index);
> >> +               break;
> >> +       case POSIX_FADV_WILLNEED:
> >> +               ret = shmem_fadvise_willneed(mapping, start_index, end_index);
> >> +               break;
> >> +       case POSIX_FADV_NORMAL:
> >> +       case POSIX_FADV_RANDOM:
> >> +       case POSIX_FADV_SEQUENTIAL:
> >> +       case POSIX_FADV_NOREUSE:
>
> --Charan