ubifs: refactor ubifs_file_mmap()

PATCH In review Zhang Zhen 2 participants 5 messages Jul 22, 2014 – Jul 29, 2014 1 message not in the archive

Subsystems (2)

Memory technology devices (MTD) UBI file system (UBIFS)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Zhang Zhen

author

2014-07-22 06:29 UTC #1 lore raw

PATCH v2 1/1 ubifs: refactor ubifs_file_mmap()

generic_file_mmap() set vma->vm_ops = &generic_file_vm_ops, then ubifs_file_mmap set vma->vm_ops = &ubifs_file_vm_ops. So it is redundant.
And there is no kind of file does not supply page reading function in ubifs. The readpage() check up for mmap file in generic_file_mmap() is not needed.

So remove the call of generic_file_mmap().

Change v1 -> v2:
- deleted the mapping variable

Signed-off-by Zhang Zhen <[email protected]>

---
 fs/ubifs/file.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

fs/ubifs/file.c +1 -5

index b5b593c..f7b6958 100644
@@ -1540,11 +1540,7 @@ static const struct vm_operations_struct ubifs_file_vm_ops = {
 
 static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
-	int err;
-
-	err = generic_file_mmap(file, vma);
-	if (err)
-		return err;
+	file_accessed(file);
 	vma->vm_ops = &ubifs_file_vm_ops;
 	return 0;
 }

Signature · 4 lines

1.8.1.2


.

Artem Bityutskiy

2014-07-28 16:25 UTC #2 lore raw

Quoted 7 lines · Zhang Zhen

generic_file_mmap() set vma->vm_ops = &generic_file_vm_ops, then ubifs_file_mmap set vma->vm_ops = &ubifs_file_vm_ops. So it is redundant.
And there is no kind of file does not supply page reading function in ubifs. The readpage() check up for mmap file in generic_file_mmap() is not needed.

Quoted 5 lines

So remove the call of generic_file_mmap().
Change v1 -> v2:
- deleted the mapping variable
Signed-off-by Zhang Zhen <[email protected]>

Hi, I am not sure about this patch. 'generic_file_mmap()' covers ! CONFIG_MMU case too, for example.

Thanks!

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Zhang Zhen

author

2014-07-29 03:51 UTC #3 lore raw

Quoted 8 lines · Artem Bityutskiy

Quoted 7 lines · Zhang Zhen
generic_file_mmap() set vma->vm_ops = &generic_file_vm_ops, then ubifs_file_mmap set vma->vm_ops = &ubifs_file_vm_ops. So it is redundant.
And there is no kind of file does not supply page reading function in ubifs. The readpage() check up for mmap file in generic_file_mmap() is not needed.

Quoted 9 lines

Quoted 5 lines
So remove the call of generic_file_mmap().
Change v1 -> v2:
- deleted the mapping variable
Signed-off-by Zhang Zhen <[email protected]>
Hi, I am not sure about this patch. 'generic_file_mmap()' covers ! CONFIG_MMU case too, for example.

Yes, I missed that. We can add #ifdef CONFIG_MMU in ubifs_file_mmap(). Considering the patch itself is not important changes, we can give up this patch, remain the same.

What do you think?

Thanks for your comments!

Quoted 2 lines

Thanks!

Artem Bityutskiy

2014-07-29 07:20 UTC #4 lore raw

Quoted 15 lines · Zhang Zhen

Quoted 14 lines · Artem Bityutskiy
Quoted 12 lines · Zhang Zhen
generic_file_mmap() set vma->vm_ops = &generic_file_vm_ops, then ubifs_file_mmap set vma->vm_ops = &ubifs_file_vm_ops. So it is redundant.
And there is no kind of file does not supply page reading function in ubifs. The readpage() check up for mmap file in generic_file_mmap() is not needed.
So remove the call of generic_file_mmap().
Change v1 -> v2:
- deleted the mapping variable
Signed-off-by Zhang Zhen <[email protected]>

Quoted 8 lines

Quoted 3 lines
Hi, I am not sure about this patch. 'generic_file_mmap()' covers ! CONFIG_MMU case too, for example.
Yes, I missed that. We can add #ifdef CONFIG_MMU in ubifs_file_mmap(). Considering the patch itself is not important changes, we can give up this patch, remain the same.
What do you think?

I think it is better to leave it as it is, because that's not too bad and works. Changing this has a risk of breaking something. UBIFS does not enjoy a huge user base like, say, ext4, so the breakage could easily go unnoticed for long time.

Signature · 2 lines

Best Regards,
Artem Bityutskiy

Zhang Zhen

author

2014-07-29 07:34 UTC #5 lore raw

Quoted 23 lines · Artem Bityutskiy

Quoted 22 lines · Zhang Zhen
Quoted 8 lines · Artem Bityutskiy
Quoted 7 lines · Zhang Zhen
generic_file_mmap() set vma->vm_ops = &generic_file_vm_ops, then ubifs_file_mmap set vma->vm_ops = &ubifs_file_vm_ops. So it is redundant.
And there is no kind of file does not supply page reading function in ubifs. The readpage() check up for mmap file in generic_file_mmap() is not needed.
Quoted 9 lines
Quoted 5 lines
So remove the call of generic_file_mmap().
Change v1 -> v2:
- deleted the mapping variable
Signed-off-by Zhang Zhen <[email protected]>
Hi, I am not sure about this patch. 'generic_file_mmap()' covers ! CONFIG_MMU case too, for example.
Yes, I missed that. We can add #ifdef CONFIG_MMU in ubifs_file_mmap(). Considering the patch itself is not important changes, we can give up this patch, remain the same.

Quoted 7 lines

Quoted 1 line
What do you think?
I think it is better to leave it as it is, because that's not too bad and works. Changing this has a risk of breaking something. UBIFS does not enjoy a huge user base like, say, ext4, so the breakage could easily go unnoticed for long time.

OK.