nvdimm/pmem: check the validity of the pointer pfn
Subsystems (6)
PATCH 1/3 nvdimm/pmem: check the validity of the pointer pfn
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from direct_access. Assigning NULL to gfn of dax_direct_access is more intuitive and simple than offering a useless local variable.
So direct_access needs to check validity of the pointer pfn For NULL assignment.
--- drivers/nvdimm/pmem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/nvdimm/pmem.c +2 -1
@@ -233,7 +233,8 @@ __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff, PFN_PHYS(nr_pages))))return -EIO ; *kaddr = pmem->virt_addr + offset; - *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags); +if (pfn) + *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags);/* * If badblocks are present, limit known good range to the
Signature · 1 line
1.8.3.1
PATCH 2/3 drivers/s390/block/dcssblk: check the validity of the pointer pfn
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from direct_access. Assigning NULL to gfn of dax_direct_access is more intuitive and simple than offering a useless local variable.
So direct_access needs to check validity of the pointer pfn For NULL assignment. If pfn equals to NULL, it doesn't need to calculate its value.
--- drivers/s390/block/dcssblk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/s390/block/dcssblk.c +2 -1
@@ -916,7 +916,8 @@ static DEVICE_ATTR(save, S_IWUSR | S_IRUSR, dcssblk_save_show, dev_sz = dev_info->end - dev_info->start +1 ; *kaddr = (void *) dev_info->start + offset; - *pfn =__pfn_to_pfn_t (PFN_DOWN (dev_info->start + offset), +if (pfn) + *pfn =__pfn_to_pfn_t (PFN_DOWN (dev_info->start + offset),PFN_DEV |PFN_SPECIAL );return (dev_sz - offset) /PAGE_SIZE ;
Signature · 1 line
1.8.3.1
PATCH 3/3 fs/dax: Assigning NULL to gfn of dax_direct_access if useless
In reply to Huaisheng YeFrom: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from direct_access. Assigning NULL to gfn of dax_direct_access is more intuitive and simple than offering a useless local variable.
--- fs/dax.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
fs/dax.c +3 -6
@@ -550,7 +550,6 @@ static int copy_user_dax(struct block_device *bdev, struct dax_device *dax_dev, {void *vto, *kaddr;pgoff_t pgoff; -pfn_t pfn;long rc;int id;
@@ -559,7 +558,7 @@ static int copy_user_dax(struct block_device *bdev, struct dax_device *dax_dev,return rc; id =dax_read_lock (); - rc =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), &kaddr, &pfn); + rc =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), &kaddr,NULL );if (rc <0 ) {dax_read_unlock (id);return rc;
@@ -961,7 +960,6 @@ int __dax_zero_page_range(struct block_device *bdev,pgoff_t pgoff;long rc, id;void *kaddr; -pfn_t pfn; rc =bdev_dax_pgoff (bdev, sector,PAGE_SIZE , &pgoff);if (rc)
@@ -969,7 +967,7 @@ int __dax_zero_page_range(struct block_device *bdev, id =dax_read_lock (); rc =dax_direct_access (dax_dev, pgoff,1 , &kaddr, - &pfn); +NULL );if (rc <0 ) {dax_read_unlock (id);return rc;
@@ -1024,7 +1022,6 @@ int __dax_zero_page_range(struct block_device *bdev,ssize_t map_len;pgoff_t pgoff;void *kaddr; -pfn_t pfn;if (fatal_signal_pending (current)) { ret = -EINTR ;
@@ -1036,7 +1033,7 @@ int __dax_zero_page_range(struct block_device *bdev,break ; map_len =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), - &kaddr, &pfn); + &kaddr,NULL );if (map_len <0 ) { ret = map_len;break ;
Signature · 1 line
1.8.3.1
Re: [PATCH 3/3] fs/dax: Assigning NULL to gfn of dax_direct_access if useless
Quoted 7 lines · Huaisheng Ye
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from
direct_access. Assigning NULL to gfn ofdax_direct_accessis more intuitive and simple than offering a useless local variable.Signed-off-by Huaisheng Ye <[email protected]>
I like this. You can add:
for the series.
Honza
Quoted 64 lines
--- fs/dax.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)fs/dax.c +3
-6@@ -550,7 +550,6 @@ static int copy_user_dax(struct block_device *bdev, struct dax_device *dax_dev, {void *vto, *kaddr;pgoff_t pgoff; -pfn_t pfn;long rc;int id;@@ -559,7 +558,7 @@ static int copy_user_dax(struct block_device *bdev, struct dax_device *dax_dev,return rc; id =dax_read_lock (); - rc =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), &kaddr, &pfn); + rc =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), &kaddr,NULL );if (rc <0 ) {dax_read_unlock (id);return rc;@@ -961,7 +960,6 @@ int __dax_zero_page_range(struct block_device *bdev,pgoff_t pgoff;long rc, id;void *kaddr; -pfn_t pfn; rc =bdev_dax_pgoff (bdev, sector,PAGE_SIZE , &pgoff);if (rc)@@ -969,7 +967,7 @@ int __dax_zero_page_range(struct block_device *bdev, id =dax_read_lock (); rc =dax_direct_access (dax_dev, pgoff,1 , &kaddr, - &pfn); +NULL );if (rc <0 ) {dax_read_unlock (id);return rc;@@ -1024,7 +1022,6 @@ int __dax_zero_page_range(struct block_device *bdev,ssize_t map_len;pgoff_t pgoff;void *kaddr; -pfn_t pfn;if (fatal_signal_pending (current)) { ret = -EINTR ;@@ -1036,7 +1033,7 @@ int __dax_zero_page_range(struct block_device *bdev,break ; map_len =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), - &kaddr, &pfn); + &kaddr,NULL );if (map_len <0 ) { ret = map_len;break ;Signature · 1 line
1.8.3.1
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 3/3] fs/dax: Assigning NULL to gfn of dax_direct_access if useless
---- On Wed, 04 Jul 2018 19:30:12 +0800 Jan Kara <[email protected]> wrote ----
Quoted 9 lines · Huaisheng Ye
Quoted 7 lines
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from
direct_access. Assigning NULL to gfn ofdax_direct_accessis more intuitive and simple than offering a useless local variable.Signed-off-by Huaisheng Ye <[email protected]>
Quoted 8 lines
I like this. You can add:
Reviewed-by Jan Kara <[email protected]>for the series.
Honza
I am so happy you like them, thank you very much.
---
Cheers,
Huaisheng
Re: [PATCH 3/3] fs/dax: Assigning NULL to gfn of dax_direct_access if useless
Quoted 12 lines · Huaisheng Ye
---- On Wed, 04 Jul 2018 19:30:12 +0800 Jan Kara <[email protected]> wrote ----
Quoted 11 lines · Huaisheng Ye
Quoted 7 lines
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from
direct_access. Assigning NULL to gfn ofdax_direct_accessis more intuitive and simple than offering a useless local variable.Signed-off-by Huaisheng Ye <[email protected]>I like this. You can add:
Quoted 7 lines
Quoted 6 lines
Reviewed-by Jan Kara <[email protected]>for the series.
Honza
I am so happy you like them, thank you very much.
Yes, I like this too. In fact I have a similar patch in my tree already that I have been preparing to send out. I am using it to delay when we need to have the 'struct page' memmap for dax initialized. Attached is the full patch, but the series is still a work in progress.
From 62e12abebacbafdb5218a92501885fe0e80ea922 Mon Sep 17 00:00:00 2001
From: Dan Williams <[email protected]>
Date: Wed, 27 Jun 2018 22:21:30 -0700
Subject: [PATCH] dax: Elide pfn lookups
When not required, allow drivers to skip returning a pfn if one was not requested. In support of asynchronous initialization of 'struct page' there needs to be a sync point at which the memmap is made valid. The latest point where the kernel can reasonably sync is when a pfn is being used to populate userspace page tables. Otherwise, if only the kernel address is needed from dax_direct_access(), typically in-kernel users, then the 'pfn_t *' argument to dax_direct_access() can be NULL.
--- drivers/dax/super.c | 17 +++++++++++------ drivers/nvdimm/pmem.c | 3 ++- drivers/s390/block/dcssblk.c | 5 +++-- fs/dax.c | 10 +++------- tools/testing/nvdimm/pmem-dax.c | 6 ++++-- 5 files changed, 23 insertions(+), 18 deletions(-)
drivers/dax/super.c +11 -6
@@ -72,7 +72,7 @@ struct dax_device *fs_dax_get_by_bdev(struct block_device *bdev)EXPORT_SYMBOL_GPL (fs_dax_get_by_bdev);#endif -static bool validate_dax_pfn (pfn_t pfn) +static bool validate_dax_pfn (pfn_t *pfn) {bool dax_enabled = false;
@@ -84,7 +84,7 @@ static bool validate_dax_pfn(pfn_t pfn)if (!IS_ENABLED (CONFIG_DAX_DRIVER_DEBUG ))return true; -if (IS_ENABLED (CONFIG_FS_DAX_LIMITED ) &&pfn_t_special (pfn)) { +if (IS_ENABLED (CONFIG_FS_DAX_LIMITED ) &&pfn_t_special (*pfn)) {/* * An arch that has enabled the pmem api should also * have its drivers support pfn_t_devmap()
@@ -95,10 +95,10 @@ static bool validate_dax_pfn(pfn_t pfn)*/ WARN_ON (IS_ENABLED (CONFIG_ARCH_HAS_PMEM_API )); dax_enabled = true; - }else if (pfn_t_devmap (pfn)) { + }else if (pfn_t_devmap (*pfn)) {struct dev_pagemap *pgmap; - pgmap =get_dev_pagemap (pfn_t_to_pfn (pfn),NULL ); + pgmap =get_dev_pagemap (pfn_t_to_pfn (*pfn),NULL );if (pgmap && pgmap->type ==MEMORY_DEVICE_FS_DAX ) dax_enabled = true;put_dev_pagemap (pgmap);
@@ -120,10 +120,10 @@ static bool validate_dax_pfn(pfn_t pfn)bool __bdev_dax_supported (struct block_device *bdev,int blocksize) {struct dax_device *dax_dev; +pfn_t _pfn, *pfn;pgoff_t pgoff;int err, id;void *kaddr; -pfn_t pfn;long len;char buf[BDEVNAME_SIZE ];
@@ -147,8 +147,13 @@ bool __bdev_dax_supported(struct block_device *bdev, int blocksize)return false; } +if (IS_ENABLED (DAX_DRIVER_DEBUG )) + pfn = &_pfn; +else + pfn =NULL ; + id =dax_read_lock (); - len =dax_direct_access (dax_dev, pgoff,1 , &kaddr, &pfn); + len =dax_direct_access (dax_dev, pgoff,1 , &kaddr, pfn);dax_read_unlock (id);put_dax (dax_dev);
drivers/nvdimm/pmem.c +2 -1
@@ -228,7 +228,8 @@ __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff, PFN_PHYS(nr_pages))))return -EIO ; *kaddr = pmem->virt_addr + offset; - *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags); +if (pfn) + *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags);/* * If badblocks are present, limit known good range to the
drivers/s390/block/dcssblk.c +3 -2
@@ -923,8 +923,9 @@ __dcssblk_direct_access(struct dcssblk_dev_info *dev_info, pgoff_t pgoff, dev_sz = dev_info->end - dev_info->start +1 ; *kaddr = (void *) dev_info->start + offset; - *pfn =__pfn_to_pfn_t (PFN_DOWN (dev_info->start + offset), -PFN_DEV |PFN_SPECIAL ); +if (pfn) + *pfn =__pfn_to_pfn_t (PFN_DOWN (dev_info->start + offset), +PFN_DEV |PFN_SPECIAL );return (dev_sz - offset) /PAGE_SIZE ; }
fs/dax.c +3 -7
@@ -647,7 +647,6 @@ static int copy_user_dax(struct block_device *bdev, struct dax_device *dax_dev, {void *vto, *kaddr;pgoff_t pgoff; -pfn_t pfn;long rc;int id;
@@ -656,7 +655,7 @@ static int copy_user_dax(struct block_device *bdev, struct dax_device *dax_dev,return rc; id =dax_read_lock (); - rc =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), &kaddr, &pfn); + rc =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), &kaddr,NULL );if (rc <0 ) {dax_read_unlock (id);return rc;
@@ -1052,15 +1051,13 @@ int __dax_zero_page_range(struct block_device *bdev,pgoff_t pgoff;long rc, id;void *kaddr; -pfn_t pfn; rc =bdev_dax_pgoff (bdev, sector,PAGE_SIZE , &pgoff);if (rc)return rc; id =dax_read_lock (); - rc =dax_direct_access (dax_dev, pgoff,1 , &kaddr, - &pfn); + rc =dax_direct_access (dax_dev, pgoff,1 , &kaddr,NULL );if (rc <0 ) {dax_read_unlock (id);return rc;
@@ -1116,7 +1113,6 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,ssize_t map_len;pgoff_t pgoff;void *kaddr; -pfn_t pfn;if (fatal_signal_pending (current)) { ret = -EINTR ;
@@ -1128,7 +1124,7 @@ dax_iomap_actor(struct inode *inode, loff_t pos, loff_t length, void *data,break ; map_len =dax_direct_access (dax_dev, pgoff,PHYS_PFN (size), - &kaddr, &pfn); + &kaddr,NULL );if (map_len <0 ) { ret = map_len;break ;
tools/testing/nvdimm/pmem-dax.c +4 -2
@@ -33,7 +33,8 @@ long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff, *kaddr = pmem->virt_addr + offset; page =vmalloc_to_page (pmem->virt_addr + offset); - *pfn =page_to_pfn_t (page); +if (pfn) + *pfn =page_to_pfn_t (page);pr_debug_ratelimited ("%s: pmem: %p pgoff: %#lx pfn: %#lx\n" , __func__, pmem, pgoff,page_to_pfn (page));
@@ -41,7 +42,8 @@ long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff, } *kaddr = pmem->virt_addr + offset; - *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags); +if (pfn) + *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags);/* * If badblocks are present, limit known good range to the
Signature · 1 line
2.13.6
Re: [PATCH 3/3] fs/dax: Assigning NULL to gfn of dax_direct_access if useless
Quoted 19 lines · Dan Williams
Quoted 18 lines · Huaisheng Ye
---- On Wed, 04 Jul 2018 19:30:12 +0800 Jan Kara <[email protected]> wrote ----
Quoted 17 lines · Huaisheng Ye
Quoted 7 lines
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from
direct_access. Assigning NULL to gfn ofdax_direct_accessis more intuitive and simple than offering a useless local variable.Signed-off-by Huaisheng Ye <[email protected]>I like this. You can add:
Reviewed-by Jan Kara <[email protected]>for the series.
Honza
Quoted 7 lines
Quoted 1 line
I am so happy you like them, thank you very much.
Yes, I like this too. In fact I have a similar patch in my tree already that I have been preparing to send out. I am using it to delay when we need to have the 'struct page' memmap for dax initialized. Attached is the full patch, but the series is still a work in progress.
Btw, I'll drop my version and apply your series since you got it posted first and it can stand alone as its own cleanup.
RE: [External] Re: [PATCH 3/3] fs/dax: Assigning NULL to gfn of dax_direct_access if useless
From: Dan Williams [mailto:[email protected]]
Sent: Wednesday, July 04, 2018 10:42 PM
Quoted 23 lines · Dan Williams
Quoted 21 lines · Huaisheng Ye
Quoted 19 lines
---- On Wed, 04 Jul 2018 19:30:12 +0800 Jan Kara <[email protected]> wrote ----
Quoted 9 lines · Huaisheng Ye
Quoted 7 lines
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from
direct_access. Assigning NULL to gfn ofdax_direct_accessis more intuitive and simple than offering a useless local variable.Signed-off-by Huaisheng Ye <[email protected]>Quoted 8 lines
I like this. You can add:
Reviewed-by Jan Kara <[email protected]>for the series.
Honza
I am so happy you like them, thank you very much.
Quoted 8 lines
Quoted 5 lines
Yes, I like this too. In fact I have a similar patch in my tree already that I have been preparing to send out. I am using it to delay when we need to have the 'struct page' memmap for dax initialized. Attached is the full patch, but the series is still a work in progress.
Btw, I'll drop my version and apply your series since you got it posted first and it can stand alone as its own cleanup.
Hi Dan,
I will resend the series later, and thanks for your help.
Cheers,
Huaisheng Ye
Quoted 19 lines · Huaisheng Ye
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from
direct_access. Assigning NULL to gfn ofdax_direct_accessis more intuitive and simple than offering a useless local variable.So
direct_accessneeds to check validity of the pointer pfn For NULL assignment.Signed-off-by Huaisheng Ye <[email protected]>--- drivers/nvdimm/pmem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/nvdimm/pmem.c +0
-0
drivers/nvdimm/pmem.c __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff, · line 240
Quoted 9 lines
drivers/nvdimm/pmem.c +2
-1@@ -233,7 +233,8 @@ __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff, PFN_PHYS(nr_pages))))return -EIO ; *kaddr = pmem->virt_addr + offset; - *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags); +if (pfn) + *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags);/* * If badblocks are present, limit known good range to the
Looks good. You also need to update the unit test infrastructure version of this operation in:
tools/testing/nvdimm/pmem-dax.c
Quoted 31 lines
From: Dan Williams [mailto:[email protected]]
Sent: Wednesday, July 04, 2018 10:40 PMdrivers/nvdimm/pmem.c __weak long __pmem_direct_access(struct pmem_device *pmem,
Quoted 19 lines · Huaisheng Ye
From: Huaisheng Ye <[email protected]>
Some functions within fs/dax don't need to get gfn from
direct_access. Assigning NULL to gfn ofdax_direct_accessis more intuitive and simple than offering a useless local variable.So
direct_accessneeds to check validity of the pointer pfn For NULL assignment.Signed-off-by Huaisheng Ye <[email protected]>--- drivers/nvdimm/pmem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/nvdimm/pmem.c +0
-0@@ -233,7 +233,8 @@ __weak long __pmem_direct_access(struct pmem_device *pmem,
pgoff_tpgoff,drivers/nvdimm/pmem.c __weak long __pmem_direct_access(struct pmem_device *pmem, pgoff_t pgoff,
Quoted 6 lines
drivers/nvdimm/pmem.c +2
-1PFN_PHYS(nr_pages))))return -EIO ; *kaddr = pmem->virt_addr + offset; - *pfn =phys_to_pfn_t (pmem->phys_addr + offset, pmem->pfn_flags); +if (pfn) + *pfn = phys_to_pfn_t(pmem->phys_addr + offset,
pmem->pfn_flags);Quoted 1 line
Quoted 7 lines
Quoted 2 lines
/*
* If badblocks are present, limit known good range to theLooks good. You also need to update the unit test infrastructure version of this operation in:
tools/testing/nvdimm/pmem-dax.c
Yes, you are right.
Cheers,
Huaisheng Ye