Message ID | 53CE049B.7050801@huawei.com |
---|---|
State | Rejected |
Headers | show |
On Tue, 2014-07-22 at 14:28 +0800, Zhang Zhen wrote: > 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 <zhenzhang.zhang@huawei.com> Hi, I am not sure about this patch. 'generic_file_mmap()' covers ! CONFIG_MMU case too, for example. Thanks!
On 2014/7/29 0:25, Artem Bityutskiy wrote: > On Tue, 2014-07-22 at 14:28 +0800, Zhang Zhen wrote: >> 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 <zhenzhang.zhang@huawei.com> > > 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! > Thanks! >
On Tue, 2014-07-29 at 11:50 +0800, Zhang Zhen wrote: > On 2014/7/29 0:25, Artem Bityutskiy wrote: > > On Tue, 2014-07-22 at 14:28 +0800, Zhang Zhen wrote: > >> 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 <zhenzhang.zhang@huawei.com> > > > > 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.
On 2014/7/29 15:20, Artem Bityutskiy wrote: > On Tue, 2014-07-29 at 11:50 +0800, Zhang Zhen wrote: >> On 2014/7/29 0:25, Artem Bityutskiy wrote: >>> On Tue, 2014-07-22 at 14:28 +0800, Zhang Zhen wrote: >>>> 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 <zhenzhang.zhang@huawei.com> >>> >>> 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. > OK.
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c index b5b593c..f7b6958 100644 --- a/fs/ubifs/file.c +++ b/fs/ubifs/file.c @@ -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; }
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 <zhenzhang.zhang@huawei.com> --- fs/ubifs/file.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)