Kirill A. Shutemov wrote:
> On Fri, Mar 16, 2018 at 10:14:24PM +0900, Tetsuo Handa wrote:
> > f2fs is doing
> >
> > page = f2fs_pagecache_get_page(inode->i_mapping, 0, FGP_LOCK|FGP_NOWAIT, 0);
> >
> > which calls
> >
> > struct page *pagecache_get_page(inode->i_mapping, 0, FGP_LOCK|FGP_NOWAIT, 0);
> >
> > . Then, can't we define
> >
> > static inline struct page *find_trylock_page(struct address_space *mapping,
> > pgoff_t offset)
> > {
> > return pagecache_get_page(mapping, offset, FGP_LOCK|FGP_NOWAIT, 0);
> > }
> >
> > and replace find_lock_page() with find_trylock_page() ?
>
> This won't work in this case. We need to destinct no-page-in-page-cache
> from failed-to-lock-page. We take different routes depending on this.
>
OK. Then, I think we should avoid reordering trylock_page() and PageTransHuge()
without patch description why it is safe. Below patch preserves the ordering
and sounds safer for stable. But either patch, please add why it is safe to omit
"/* Has the page been truncated? */" check which would have been done for FGP_LOCK
in patch description.
---
mm/shmem.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 8ead6cb..5e94ca4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -493,16 +493,27 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
info = list_entry(pos, struct shmem_inode_info, shrinklist);
inode = &info->vfs_inode;
- if (nr_to_split && split >= nr_to_split) {
- iput(inode);
- continue;
- }
+ if (nr_to_split && split >= nr_to_split)
+ goto leave;
- page = find_lock_page(inode->i_mapping,
+ page = find_get_page(inode->i_mapping,
(inode->i_size & HPAGE_PMD_MASK) >> PAGE_SHIFT);
if (!page)
goto drop;
+ /*
+ * Leave the inode on the list if we failed to lock
+ * the page at this time.
+ *
+ * Waiting for the lock may lead to deadlock in the
+ * reclaim path.
+ */
+ if (!trylock_page(page)) {
+ put_page(page);
+ goto leave;
+ }
+
+ /* No huge page at the end of the file: nothing to split */
if (!PageTransHuge(page)) {
unlock_page(page);
put_page(page);
@@ -513,16 +524,15 @@ static unsigned long shmem_unused_huge_shrink(struct shmem_sb_info *sbinfo,
unlock_page(page);
put_page(page);
- if (ret) {
- /* split failed: leave it on the list */
- iput(inode);
- continue;
- }
+ /* If split failed leave the inode on the list */
+ if (ret)
+ goto leave;
split++;
drop:
list_del_init(&info->shrinklist);
removed++;
+leave:
iput(inode);
}
--