[PATCH V6 0/2] mm: shmem: support POSIX_FADV_[WILL|DONT]NEED for shmem files

[PATCH V6 0/2] mm: shmem: support POSIX_FADV_[WILL|DONT]NEED for shmem files

[Charan Teja Kalla]

This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
advices to shmem files which can be helpful for the drivers who may want
to manage the pages of shmem files on their own, like, that are created
through shmem_file_setup[_with_mnt]().

Changes in V6:
 -- Replaced the pages with folio's for shmem changes.

Changes in V5:
 -- Moved the 'endbyte' calculations to a header function for use by shmem_fadvise().
 -- Addressed comments from suren.
 -- No changes in resend. Retested on the latest tip.
 -- https://lore.kernel.org/all/cover.1648706231.git.quic_charante@quicinc.com/

Changes in V4:
  -- Changed the code to use reclaim_pages() to writeout the shmem pages to swap and then reclaim.
  -- Addressed comments from Mark Hemment and Matthew.
  -- fadvise() on shmem file may even unmap a page.
  -- https://patchwork.kernel.org/project/linux-mm/patch/1644572051-24091-1-git-send-email-quic_charante@quicinc.com/

Changes in V3:
  -- Considered THP pages while doing FADVISE_[DONT|WILL]NEED, identified by Matthew.
  -- xarray used properly, as identified by Matthew.
  -- Excluded mapped pages as it requires unmapping and the man pages of fadvise don't talk about them.
  -- RESEND: Fixed the compilation issue when CONFIG_TMPFS is not defined.
  -- https://patchwork.kernel.org/project/linux-mm/patch/1641488717-13865-1-git-send-email-quic_charante@quicinc.com/

Changes in V2:
  -- Rearranged the code to not to sleep with rcu_lock while using xas_() functionality.
  -- Addressed the comments from Suren.
  -- https://patchwork.kernel.org/project/linux-mm/patch/1638442253-1591-1-git-send-email-quic_charante@quicinc.com/

changes in V1:
  -- Created the interface for fadvise(2) to work on shmem files.
  -- https://patchwork.kernel.org/project/linux-mm/patch/1633701982-22302-1-git-send-email-charante@codeaurora.org/


Charan Teja Kalla (2):
  mm: fadvise: move 'endbyte' calculations to helper function
  mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem

 mm/fadvise.c  |  11 +-----
 mm/internal.h |  21 +++++++++++
 mm/shmem.c    | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH V6 1/2] mm: fadvise: move 'endbyte' calculations to helper function

[Charan Teja Kalla]

Move the 'endbyte' calculations that determines last byte that fadvise
can to a helper function. This is a preparatory change made for
shmem_fadvise() functionality in the next patch. No functional changes
in this patch.

Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
---
 mm/fadvise.c  | 11 +----------
 mm/internal.h | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/mm/fadvise.c b/mm/fadvise.c
index bf04fec..10169c6 100644
--- a/mm/fadvise.c
+++ b/mm/fadvise.c
@@ -65,16 +65,7 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 		return 0;
 	}
 
-	/*
-	 * Careful about overflows. Len == 0 means "as much as possible".  Use
-	 * unsigned math because signed overflows are undefined and UBSan
-	 * complains.
-	 */
-	endbyte = (u64)offset + (u64)len;
-	if (!len || endbyte < len)
-		endbyte = LLONG_MAX;
-	else
-		endbyte--;		/* inclusive */
+	endbyte = fadvise_calc_endbyte(offset, len);
 
 	switch (advice) {
 	case POSIX_FADV_NORMAL:
diff --git a/mm/internal.h b/mm/internal.h
index bcf75a8..47858af 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -637,6 +637,27 @@ static inline void vunmap_range_noflush(unsigned long start, unsigned long end)
 }
 #endif /* !CONFIG_MMU */
 
+/*
+ * Helper function to get the endbyte of a file that fadvise can operate on.
+ */
+static inline loff_t fadvise_calc_endbyte(loff_t offset, loff_t len)
+{
+	loff_t endbyte;
+
+	/*
+	 * Careful about overflows. Len == 0 means "as much as possible".  Use
+	 * unsigned math because signed overflows are undefined and UBSan
+	 * complains.
+	 */
+	endbyte = (u64)offset + (u64)len;
+	if (!len || endbyte < len)
+		endbyte = LLONG_MAX;
+	else
+		endbyte--;		/* inclusive */
+
+	return endbyte;
+}
+
 /* Memory initialisation debug and verification */
 enum mminit_level {
 	MMINIT_WARNING,
-- 
2.7.4

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

[Charan Teja Kalla]

Currently fadvise(2) is supported only for the files that doesn't
associated with noop_backing_dev_info thus for the files, like shmem,
fadvise results into NOP. But then there is file_operations->fadvise()
that lets the file systems to implement their own fadvise
implementation. Use this support to implement some of the POSIX_FADV_XXX
functionality for shmem files.

This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
advices to shmem files which can be helpful for the clients who may want
to manage the shmem pages of the files that are created through
shmem_file_setup[_with_mnt](). One usecase is implemented on the
Snapdragon SoC's running Android where the graphics client is allocating
lot of shmem pages per process and pinning them. When this process is
put to background, the instantaneous reclaim is performed on those shmem
pages using the logic implemented downstream[3][4]. With this patch, the
client can now issue the fadvise calls on the shmem files that does the
instantaneous reclaim which can aid the use cases like mentioned above.

This usecase lead to ~2% reduction in average launch latencies of the
apps and 10% in total number of kills by the low memory killer running
on Android.

Some questions asked while reviewing this patch:
Q) Can the same thing be achieved with FD mapped to user and use
madvise?
A) All drivers are not mapping all the shmem fd's to user space and want
to manage them with in the kernel. Ex: shmem memory can be mapped to the
other subsystems and they fill in the data and then give it to other
subsystem for further processing, where, the user mapping is not at all
required.  A simple example, memory that is given for gpu subsystem
which can be filled directly and give to display subsystem. And the
respective drivers know well about when to keep that memory in ram or
swap based on may be a user activity.

Q) Should we add the documentation section in Manual pages?
A) The man[1] pages for the fadvise() whatever says is also applicable
for shmem files. so couldn't feel it correct to add specific to shmem
files separately.

Q) The proposed semantics of POSIX_FADV_DONTNEED is actually similar to
MADV_PAGEOUT and different from MADV_DONTNEED. This is a user facing API
and this difference will cause confusion?
A) man pages [2] says that "POSIX_FADV_DONTNEED attempts to free cached
pages associated with the specified region." This means on issuing this
FADV, it is expected to free the file cache pages. And it is
implementation defined If the dirty pages may be attempted to writeback.
And the unwritten dirty pages will not be freed. So, FADV_DONTNEED also
covers the semantics of MADV_PAGEOUT for file pages and there is no
purpose of PAGEOUT for file pages.

[1] https://linux.die.net/man/2/fadvise
[2] https://man7.org/linux/man-pages/man2/posix_fadvise.2.html
[3] https://git.codelinaro.org/clo/la/platform/vendor/qcom/opensource/graphics-kernel/-/blob/gfx-kernel.lnx.1.0.r3-rel/kgsl_reclaim.c#L289
[4] https://android.googlesource.com/kernel/common/+/refs/heads/android12-5.10/mm/shmem.c#4310

Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
---
 mm/shmem.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/mm/shmem.c b/mm/shmem.c
index 0005ab2..58aa3d7 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -39,6 +39,9 @@
 #include <linux/fs_parser.h>
 #include <linux/swapfile.h>
 #include <linux/iversion.h>
+#include <linux/mm_inline.h>
+#include <linux/fadvise.h>
+#include <linux/page_idle.h>
 #include "swap.h"
 
 static struct vfsmount *shm_mnt;
@@ -2327,6 +2330,117 @@ static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags)
 #define shmem_initxattrs NULL
 #endif
 
+static void shmem_isolate_pages_range(struct address_space *mapping, loff_t start,
+				loff_t end, struct list_head *list)
+{
+	XA_STATE(xas, &mapping->i_pages, start);
+	struct folio *folio;
+
+	rcu_read_lock();
+	xas_for_each(&xas, folio, end) {
+		if (xas_retry(&xas, folio))
+			continue;
+		if (xa_is_value(folio))
+			continue;
+
+		if (!folio_try_get(folio))
+			continue;
+		if (folio_test_unevictable(folio) || folio_mapped(folio) ||
+				folio_isolate_lru(folio)) {
+			folio_put(folio);
+			continue;
+		}
+		folio_put(folio);
+
+		/*
+		 * Prepare the page to be passed to the reclaim_pages().
+		 * VM couldn't reclaim the page unless we clear PG_young.
+		 */
+		folio_clear_referenced(folio);
+		folio_test_clear_young(folio);
+		list_add(&folio->lru, list);
+		if (need_resched()) {
+			xas_pause(&xas);
+			cond_resched_rcu();
+		}
+	}
+	rcu_read_unlock();
+}
+
+static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,
+				loff_t end)
+{
+	LIST_HEAD(folio_list);
+
+	if (!total_swap_pages)
+		return 0;
+
+	lru_add_drain();
+	shmem_isolate_pages_range(mapping, start, end, &folio_list);
+	reclaim_pages(&folio_list);
+
+	return 0;
+}
+
+static int shmem_fadvise_willneed(struct address_space *mapping,
+				 pgoff_t start, pgoff_t long end)
+{
+	struct page *page;
+	pgoff_t index;
+
+	xa_for_each_range(&mapping->i_pages, index, page, start, end) {
+		if (!xa_is_value(page))
+			continue;
+		page = shmem_read_mapping_page(mapping, index);
+		if (!IS_ERR(page))
+			put_page(page);
+	}
+
+	return 0;
+}
+
+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:
+		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:
+		/*
+		 * No bad return value, but ignore advice.
+		 */
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 static struct inode *shmem_get_inode(struct super_block *sb, struct inode *dir,
 				     umode_t mode, dev_t dev, unsigned long flags)
 {
@@ -3933,6 +4047,7 @@ static const struct file_operations shmem_file_operations = {
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= shmem_fallocate,
 #endif
+	.fadvise	= shmem_fadvise,
 };
 
 static const struct inode_operations shmem_inode_operations = {
-- 
2.7.4

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

[Matthew Wilcox]

On Mon, Feb 06, 2023 at 07:51:33PM +0530, Charan Teja Kalla wrote:
> +static int shmem_fadvise_willneed(struct address_space *mapping,
> +				 pgoff_t start, pgoff_t long end)
> +{
> +	struct page *page;
> +	pgoff_t index;
> +
> +	xa_for_each_range(&mapping->i_pages, index, page, start, end) {
> +		if (!xa_is_value(page))
> +			continue;
> +		page = shmem_read_mapping_page(mapping, index);
> +		if (!IS_ERR(page))
> +			put_page(page);
> +	}
> +
> +	return 0;
> +}

Hm, that's a gap in the shmem folio API.  Patches imminent.

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

[Charan Teja Kalla]

Thanks Matthew!!

On 2/6/2023 9:48 PM, Matthew Wilcox wrote:
>> +static int shmem_fadvise_willneed(struct address_space *mapping,
>> +				 pgoff_t start, pgoff_t long end)
>> +{
>> +	struct page *page;
>> +	pgoff_t index;
>> +
>> +	xa_for_each_range(&mapping->i_pages, index, page, start, end) {
>> +		if (!xa_is_value(page))
>> +			continue;
>> +		page = shmem_read_mapping_page(mapping, index);
>> +		if (!IS_ERR(page))
>> +			put_page(page);
>> +	}
>> +
>> +	return 0;
>> +}
> Hm, that's a gap in the shmem folio API.  Patches imminent.
I will change this piece of code to folios based on your recent set of
patches. Will wait for more reviews before update.

Thanks,
Charan

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

[Suren Baghdasaryan]

Hi Charan,

On Mon, Feb 6, 2023 at 6:22 AM Charan Teja Kalla
<quic_charante@quicinc.com> wrote:
>
> Currently fadvise(2) is supported only for the files that doesn't
> associated with noop_backing_dev_info thus for the files, like shmem,
> fadvise results into NOP. But then there is file_operations->fadvise()
> that lets the file systems to implement their own fadvise
> implementation. Use this support to implement some of the POSIX_FADV_XXX
> functionality for shmem files.
>
> This patch aims to implement POSIX_FADV_WILLNEED and POSIX_FADV_DONTNEED
> advices to shmem files which can be helpful for the clients who may want
> to manage the shmem pages of the files that are created through
> shmem_file_setup[_with_mnt](). One usecase is implemented on the
> Snapdragon SoC's running Android where the graphics client is allocating
> lot of shmem pages per process and pinning them. When this process is
> put to background, the instantaneous reclaim is performed on those shmem
> pages using the logic implemented downstream[3][4]. With this patch, the

Thanks for upstreaming this feature!

> client can now issue the fadvise calls on the shmem files that does the
> instantaneous reclaim which can aid the use cases like mentioned above.
>
> This usecase lead to ~2% reduction in average launch latencies of the
> apps and 10% in total number of kills by the low memory killer running
> on Android.
>
> Some questions asked while reviewing this patch:
> Q) Can the same thing be achieved with FD mapped to user and use
> madvise?
> A) All drivers are not mapping all the shmem fd's to user space and want
> to manage them with in the kernel. Ex: shmem memory can be mapped to the
> other subsystems and they fill in the data and then give it to other
> subsystem for further processing, where, the user mapping is not at all
> required.  A simple example, memory that is given for gpu subsystem
> which can be filled directly and give to display subsystem. And the
> respective drivers know well about when to keep that memory in ram or
> swap based on may be a user activity.
>
> Q) Should we add the documentation section in Manual pages?
> A) The man[1] pages for the fadvise() whatever says is also applicable
> for shmem files. so couldn't feel it correct to add specific to shmem
> files separately.
>
> Q) The proposed semantics of POSIX_FADV_DONTNEED is actually similar to
> MADV_PAGEOUT and different from MADV_DONTNEED. This is a user facing API
> and this difference will cause confusion?
> A) man pages [2] says that "POSIX_FADV_DONTNEED attempts to free cached
> pages associated with the specified region." This means on issuing this
> FADV, it is expected to free the file cache pages. And it is
> implementation defined If the dirty pages may be attempted to writeback.
> And the unwritten dirty pages will not be freed. So, FADV_DONTNEED also
> covers the semantics of MADV_PAGEOUT for file pages and there is no
> purpose of PAGEOUT for file pages.
>
> [1] https://linux.die.net/man/2/fadvise
> [2] https://man7.org/linux/man-pages/man2/posix_fadvise.2.html
> [3] https://git.codelinaro.org/clo/la/platform/vendor/qcom/opensource/graphics-kernel/-/blob/gfx-kernel.lnx.1.0.r3-rel/kgsl_reclaim.c#L289
> [4] https://android.googlesource.com/kernel/common/+/refs/heads/android12-5.10/mm/shmem.c#4310
>
> Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>
> ---
>  mm/shmem.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 0005ab2..58aa3d7 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -39,6 +39,9 @@
>  #include <linux/fs_parser.h>
>  #include <linux/swapfile.h>
>  #include <linux/iversion.h>
> +#include <linux/mm_inline.h>
> +#include <linux/fadvise.h>
> +#include <linux/page_idle.h>
>  #include "swap.h"
>
>  static struct vfsmount *shm_mnt;
> @@ -2327,6 +2330,117 @@ static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags)
>  #define shmem_initxattrs NULL
>  #endif
>
> +static void shmem_isolate_pages_range(struct address_space *mapping, loff_t start,
> +                               loff_t end, struct list_head *list)
> +{
> +       XA_STATE(xas, &mapping->i_pages, start);
> +       struct folio *folio;
> +
> +       rcu_read_lock();
> +       xas_for_each(&xas, folio, end) {
> +               if (xas_retry(&xas, folio))
> +                       continue;
> +               if (xa_is_value(folio))
> +                       continue;
> +
> +               if (!folio_try_get(folio))
> +                       continue;
> +               if (folio_test_unevictable(folio) || folio_mapped(folio) ||
> +                               folio_isolate_lru(folio)) {
> +                       folio_put(folio);
> +                       continue;
> +               }
> +               folio_put(folio);
> +
> +               /*
> +                * Prepare the page to be passed to the reclaim_pages().
> +                * VM couldn't reclaim the page unless we clear PG_young.

nit: Since you operate on folios now, should you update this comment as well?

> +                */
> +               folio_clear_referenced(folio);
> +               folio_test_clear_young(folio);
> +               list_add(&folio->lru, list);
> +               if (need_resched()) {
> +                       xas_pause(&xas);
> +                       cond_resched_rcu();
> +               }
> +       }
> +       rcu_read_unlock();
> +}
> +
> +static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,
> +                               loff_t end)
> +{
> +       LIST_HEAD(folio_list);
> +
> +       if (!total_swap_pages)
> +               return 0;
> +
> +       lru_add_drain();
> +       shmem_isolate_pages_range(mapping, start, end, &folio_list);
> +       reclaim_pages(&folio_list);
> +
> +       return 0;
> +}
> +
> +static int shmem_fadvise_willneed(struct address_space *mapping,
> +                                pgoff_t start, pgoff_t long end)
> +{
> +       struct page *page;
> +       pgoff_t index;
> +
> +       xa_for_each_range(&mapping->i_pages, index, page, start, end) {
> +               if (!xa_is_value(page))
> +                       continue;
> +               page = shmem_read_mapping_page(mapping, index);
> +               if (!IS_ERR(page))
> +                       put_page(page);
> +       }
> +
> +       return 0;
> +}
> +
> +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?

> +               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:
> +               /*
> +                * No bad return value, but ignore advice.
> +                */
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return ret;
> +}
> +
>  static struct inode *shmem_get_inode(struct super_block *sb, struct inode *dir,
>                                      umode_t mode, dev_t dev, unsigned long flags)
>  {
> @@ -3933,6 +4047,7 @@ static const struct file_operations shmem_file_operations = {
>         .splice_write   = iter_file_splice_write,
>         .fallocate      = shmem_fallocate,
>  #endif
> +       .fadvise        = shmem_fadvise,
>  };
>
>  static const struct inode_operations shmem_inode_operations = {
> --
> 2.7.4
>

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

[Charan Teja Kalla]

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.

>> +               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

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@quicinc.com> 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