Skip to contentSkip to conversation

mm: Don't bother dropping mmap_sem for zero size readahead

PATCH Applied Jan Kara 5 participants 5 messages Feb 12, 2020 – Aug 1, 2020

Subsystems (3)
Stable branch Memory management Page cache
lore
Share & tools
lore

Jan Kara

author maintainer
#0 lore raw

PATCH 1/1 mm: Don't bother dropping mmap_sem for zero size readahead

When handling a page fault, we drop mmap_sem to start async readahead so that we don't block on IO submission with mmap_sem held. However there's no point to drop mmap_sem in case readahead is disabled. Handle that case to avoid pointless dropping of mmap_sem and retrying the fault. This was actually reported to block mlockall(MCL_CURRENT) indefinitely.

Fixes 6b4c9f446981 ("filemap: drop the mmap_sem for all blocking operations")
Reported-by Minchan Kim <[email protected]>
Reported-by Robert Stupp <[email protected]>
Signed-off-by Jan Kara <[email protected]>
---
 mm/filemap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Andrew, could you please pick up this patch? Minchan also tripped over this bug...

mm/filemap.c +1 -1
index 1146fcfa3215..3d39c437b07e 100644
@@ -2458,7 +2458,7 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf,
 	pgoff_t offset = vmf->pgoff;
 
 	/* If we don't want any read-ahead, don't bother */
-	if (vmf->vma->vm_flags & VM_RAND_READ)
+	if (vmf->vma->vm_flags & VM_RAND_READ || !ra->ra_pages)
 		return fpin;
 	if (ra->mmap_miss > 0)
 		ra->mmap_miss--;
Signature · 1 line
2.16.4
Reviewed-by on v1 1/1
#1 lore raw
Quoted 11 lines · Jan Kara

When handling a page fault, we drop mmap_sem to start async readahead so that we don't block on IO submission with mmap_sem held. However there's no point to drop mmap_sem in case readahead is disabled. Handle that case to avoid pointless dropping of mmap_sem and retrying the fault. This was actually reported to block mlockall(MCL_CURRENT) indefinitely.

Fixes 6b4c9f446981 ("filemap: drop the mmap_sem for all blocking operations")
Reported-by Minchan Kim <[email protected]>
Reported-by Robert Stupp <[email protected]>
Signed-off-by Jan Kara <[email protected]>
Reviewed-by Josef Bacik <[email protected]>

Thanks,

Josef

#2 lore raw
In reply to Jan Kara
Quoted 11 lines · Jan Kara

When handling a page fault, we drop mmap_sem to start async readahead so that we don't block on IO submission with mmap_sem held. However there's no point to drop mmap_sem in case readahead is disabled. Handle that case to avoid pointless dropping of mmap_sem and retrying the fault. This was actually reported to block mlockall(MCL_CURRENT) indefinitely.

Fixes 6b4c9f446981 ("filemap: drop the mmap_sem for all blocking operations")
Reported-by Minchan Kim <[email protected]>
Reported-by Robert Stupp <[email protected]>
Signed-off-by Jan Kara <[email protected]>
Reveiewed-by Minchan Kim <[email protected]>
#3 lore raw
In reply to Jan Kara

Hello,

The commit fixed by this patch[1] was merged in v5.1 and this patch was merged in the mainline in v5.7 (5c72feee3e45b40a3c96c7145ec422899d0e8964). Thus, the issue affects [v5.1, v5.6]. I was also able to reproduce the issue and confirm the fix works on v5.4 based kernels.

However, I couldn't find this fix in neither latest stable/linux-5.4.y, nor stable-queue/master. Could you please put this patch in the queue?

[1] https://lore.kernel.org/linux-mm/[email protected]/

Thanks,
SeongJae Park

Quoted 33 lines · On Wed, 12 Feb 2020 11:13:56 +0100 Jan Kara <[email protected]> wrote:

When handling a page fault, we drop mmap_sem to start async readahead so that we don't block on IO submission with mmap_sem held. However there's no point to drop mmap_sem in case readahead is disabled. Handle that case to avoid pointless dropping of mmap_sem and retrying the fault. This was actually reported to block mlockall(MCL_CURRENT) indefinitely.

Fixes 6b4c9f446981 ("filemap: drop the mmap_sem for all blocking operations")
Reported-by Minchan Kim <[email protected]>
Reported-by Robert Stupp <[email protected]>
Signed-off-by Jan Kara <[email protected]>
---
 mm/filemap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Andrew, could you please pick up this patch? Minchan also tripped over this bug...

mm/filemap.c +1 -1
index 1146fcfa3215..3d39c437b07e 100644
@@ -2458,7 +2458,7 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf,
 	pgoff_t offset = vmf->pgoff;
 
 	/* If we don't want any read-ahead, don't bother */
-	if (vmf->vma->vm_flags & VM_RAND_READ)
+	if (vmf->vma->vm_flags & VM_RAND_READ || !ra->ra_pages)
 		return fpin;
 	if (ra->mmap_miss > 0)
 		ra->mmap_miss--;
Signature · 1 line
2.16.4
maintainer
#4 lore raw
Quoted 8 lines · SeongJae Park

Hello,

The commit fixed by this patch[1] was merged in v5.1 and this patch was merged in the mainline in v5.7 (5c72feee3e45b40a3c96c7145ec422899d0e8964). Thus, the issue affects [v5.1, v5.6]. I was also able to reproduce the issue and confirm the fix works on v5.4 based kernels.

Quoted 4 lines

However, I couldn't find this fix in neither latest stable/linux-5.4.y, nor stable-queue/master. Could you please put this patch in the queue?

[1] https://lore.kernel.org/linux-mm/[email protected]/

Now queued up, thanks.

greg k-h