Re: [PATCH v7 1/8] dax: Use rwsem for dax_{read,write}_lock()
在 2021/10/15 1:48, Darrick J. Wong 写道:
On Fri, Sep 24, 2021 at 09:09:52PM +0800, Shiyang Ruan wrote:
In order to introduce dax holder registration, we need a write lock for
dax. Because of the rarity of notification failures and the infrequency
of registration events, it would be better to be a global lock rather
than per-device. So, change the current lock to rwsem and introduce a
write lock for registration.
Urgh, I totally thought dax_read_lock was a global lock on something
relating to the global dax_device state until I noticed this comment
above kill_dax():
/*
* Note, rcu is not protecting the liveness of dax_dev, rcu is ensuring
* that any fault handlers or operations that might have seen
* dax_alive(), have completed. Any operations that start after
* synchronize_srcu() has run will abort upon seeing !dax_alive().
*/
So dax_srcu ensures stability in the dax_device's ALIVE state while any
code that relies on that aliveness runs. As a side effect, it'll block
kill_dax (and I guess run_dax) while those functions run. It doesn't
protect any global state at all... but this isn't made obvious in the
API by (for example) passing the dax_device into dax_read_lock.
IOWs, It's not protecting against the dax_device getting freed or
anything resembling global state. So that's probably why you note above
that this /could/ be a per-device synchronization primitive, right?
If that's the case, then why shouldn't this be a per-device item? As
written here, any code that takes dax_write_lock() will block every dax
device in the system while it does some work on a single dax device.
Being an rwsem, it will also have to wait for every other dax device
access to complete before it can begin. That seems excessive,
particularly if in the future we start hooking up lots of pmem to a
single host.
I have more to say around kill_dax() below.
Signed-off-by: Shiyang Ruan
---
drivers/dax/device.c | 11 +-
drivers/dax/super.c| 43 ++
drivers/md/dm-writecache.c | 7 +++
fs/dax.c | 26 +++
include/linux/dax.h| 9
5 files changed, 49 insertions(+), 47 deletions(-)
diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index dd8222a42808..cc7b835509f9 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -198,7 +198,6 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
struct file *filp = vmf->vma->vm_file;
unsigned long fault_size;
vm_fault_t rc = VM_FAULT_SIGBUS;
- int id;
pfn_t pfn;
struct dev_dax *dev_dax = filp->private_data;
@@ -206,7 +205,7 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
(vmf->flags & FAULT_FLAG_WRITE) ? "write" : "read",
vmf->vma->vm_start, vmf->vma->vm_end, pe_size);
- id = dax_read_lock();
+ dax_read_lock();
switch (pe_size) {
case PE_SIZE_PTE:
fault_size = PAGE_SIZE;
@@ -246,7 +245,7 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
page->index = pgoff + i;
}
}
- dax_read_unlock(id);
+ dax_read_unlock();
return rc;
}
@@ -284,7 +283,7 @@ static const struct vm_operations_struct dax_vm_ops = {
static int dax_mmap(struct file *filp, struct vm_area_struct *vma)
{
struct dev_dax *dev_dax = filp->private_data;
- int rc, id;
+ int rc;
dev_dbg(_dax->dev, "trace\n");
@@ -292,9 +291,9 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma)
* We lock to check dax_dev liveness and will re-check at
* fault time.
*/
- id = dax_read_lock();
+ dax_read_lock();
rc = check_vma(dev_dax, vma, __func__);
- dax_read_unlock(id);
+ dax_read_unlock();
if (rc)
return rc;
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index fc89e91beea7..48ce86501d93 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -36,7 +36,7 @@ struct dax_device {
};
static dev_t dax_devt;
-DEFINE_STATIC_SRCU(dax_srcu);
+static DECLARE_RWSEM(dax_rwsem);
static struct vfsmount *dax_mnt;
static DEFINE_IDA(dax_minor_ida);
static struct kmem_cache *dax_cache __read_mostly;
@@ -46,18 +46,28 @@ static struct super_block *dax_superblock __read_mostly;
static struct hlist_head dax_host_list[DAX_HASH_SIZE];
static DEFINE_SPINLOCK(dax_host_lock);
-int dax_read_lock(void)
+void dax_read_lock(void)
{
- return srcu_read_lock(_srcu);
+ down_read(_rwsem);
}
EXPORT_SYMBOL_GPL(dax_read_lock);
-void dax_read_unlock(int id)
+void dax_read_unlock(void)
{
- srcu_read_unlock(_srcu, id);
+ up_read(_rwsem);
}
EXPORT_SYMBOL_GPL(dax_read_unlock);
+void dax_write_lock(void)
+{
+ down_write(_rwsem);
+}
+
+void dax_write_unlock(void)
+{
+
Re: [PATCH v7 1/8] dax: Use rwsem for dax_{read,write}_lock()
On Fri, Sep 24, 2021 at 09:09:52PM +0800, Shiyang Ruan wrote: > In order to introduce dax holder registration, we need a write lock for > dax. Because of the rarity of notification failures and the infrequency > of registration events, it would be better to be a global lock rather > than per-device. So, change the current lock to rwsem and introduce a > write lock for registration. I don't think taking the rw_semaphore everywhere will scale, as basically any DAX based I/O will take it (in read mode). So at a minimum we'd need a per-device percpu_rw_semaphore if we want to go there.
Re: [PATCH v7 1/8] dax: Use rwsem for dax_{read,write}_lock()
On Fri, Sep 24, 2021 at 09:09:52PM +0800, Shiyang Ruan wrote:
> In order to introduce dax holder registration, we need a write lock for
> dax. Because of the rarity of notification failures and the infrequency
> of registration events, it would be better to be a global lock rather
> than per-device. So, change the current lock to rwsem and introduce a
> write lock for registration.
Urgh, I totally thought dax_read_lock was a global lock on something
relating to the global dax_device state until I noticed this comment
above kill_dax():
/*
* Note, rcu is not protecting the liveness of dax_dev, rcu is ensuring
* that any fault handlers or operations that might have seen
* dax_alive(), have completed. Any operations that start after
* synchronize_srcu() has run will abort upon seeing !dax_alive().
*/
So dax_srcu ensures stability in the dax_device's ALIVE state while any
code that relies on that aliveness runs. As a side effect, it'll block
kill_dax (and I guess run_dax) while those functions run. It doesn't
protect any global state at all... but this isn't made obvious in the
API by (for example) passing the dax_device into dax_read_lock.
IOWs, It's not protecting against the dax_device getting freed or
anything resembling global state. So that's probably why you note above
that this /could/ be a per-device synchronization primitive, right?
If that's the case, then why shouldn't this be a per-device item? As
written here, any code that takes dax_write_lock() will block every dax
device in the system while it does some work on a single dax device.
Being an rwsem, it will also have to wait for every other dax device
access to complete before it can begin. That seems excessive,
particularly if in the future we start hooking up lots of pmem to a
single host.
I have more to say around kill_dax() below.
> Signed-off-by: Shiyang Ruan
> ---
> drivers/dax/device.c | 11 +-
> drivers/dax/super.c| 43 ++
> drivers/md/dm-writecache.c | 7 +++
> fs/dax.c | 26 +++
> include/linux/dax.h| 9
> 5 files changed, 49 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index dd8222a42808..cc7b835509f9 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -198,7 +198,6 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
> struct file *filp = vmf->vma->vm_file;
> unsigned long fault_size;
> vm_fault_t rc = VM_FAULT_SIGBUS;
> - int id;
> pfn_t pfn;
> struct dev_dax *dev_dax = filp->private_data;
>
> @@ -206,7 +205,7 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
> (vmf->flags & FAULT_FLAG_WRITE) ? "write" : "read",
> vmf->vma->vm_start, vmf->vma->vm_end, pe_size);
>
> - id = dax_read_lock();
> + dax_read_lock();
> switch (pe_size) {
> case PE_SIZE_PTE:
> fault_size = PAGE_SIZE;
> @@ -246,7 +245,7 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
> page->index = pgoff + i;
> }
> }
> - dax_read_unlock(id);
> + dax_read_unlock();
>
> return rc;
> }
> @@ -284,7 +283,7 @@ static const struct vm_operations_struct dax_vm_ops = {
> static int dax_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> struct dev_dax *dev_dax = filp->private_data;
> - int rc, id;
> + int rc;
>
> dev_dbg(_dax->dev, "trace\n");
>
> @@ -292,9 +291,9 @@ static int dax_mmap(struct file *filp, struct
> vm_area_struct *vma)
>* We lock to check dax_dev liveness and will re-check at
>* fault time.
>*/
> - id = dax_read_lock();
> + dax_read_lock();
> rc = check_vma(dev_dax, vma, __func__);
> - dax_read_unlock(id);
> + dax_read_unlock();
> if (rc)
> return rc;
>
> diff --git a/drivers/dax/super.c b/drivers/dax/super.c
> index fc89e91beea7..48ce86501d93 100644
> --- a/drivers/dax/super.c
> +++ b/drivers/dax/super.c
> @@ -36,7 +36,7 @@ struct dax_device {
> };
>
> static dev_t dax_devt;
> -DEFINE_STATIC_SRCU(dax_srcu);
> +static DECLARE_RWSEM(dax_rwsem);
> static struct vfsmount *dax_mnt;
> static DEFINE_IDA(dax_minor_ida);
> static struct kmem_cache *dax_cache __read_mostly;
> @@ -46,18 +46,28 @@ static struct super_block *dax_superblock __read_mostly;
> static struct hlist_head dax_host_list[DAX_HASH_SIZE];
> static DEFINE_SPINLOCK(dax_host_lock);
>
> -int dax_read_lock(void)
> +void dax_read_lock(void)
> {
> - return srcu_read_lock(_srcu);
> + down_read(_rwsem);
> }
> EXPORT_SYMBOL_GPL(dax_read_lock);
>
> -void dax_read_unlock(int id)
> +void dax_read_unlock(void)
> {
> - srcu_read_unlock(_srcu, id);
> + up_read(_rwsem);
> }
> EXPORT_SYMBOL_GPL(dax_read_unlock);
>
> +void dax_write_lock(void)
> +{
> + down_write(_rwsem);
> +}
[PATCH v7 1/8] dax: Use rwsem for dax_{read,write}_lock()
In order to introduce dax holder registration, we need a write lock for
dax. Because of the rarity of notification failures and the infrequency
of registration events, it would be better to be a global lock rather
than per-device. So, change the current lock to rwsem and introduce a
write lock for registration.
Signed-off-by: Shiyang Ruan
---
drivers/dax/device.c | 11 +-
drivers/dax/super.c| 43 ++
drivers/md/dm-writecache.c | 7 +++
fs/dax.c | 26 +++
include/linux/dax.h| 9
5 files changed, 49 insertions(+), 47 deletions(-)
diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index dd8222a42808..cc7b835509f9 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -198,7 +198,6 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
struct file *filp = vmf->vma->vm_file;
unsigned long fault_size;
vm_fault_t rc = VM_FAULT_SIGBUS;
- int id;
pfn_t pfn;
struct dev_dax *dev_dax = filp->private_data;
@@ -206,7 +205,7 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
(vmf->flags & FAULT_FLAG_WRITE) ? "write" : "read",
vmf->vma->vm_start, vmf->vma->vm_end, pe_size);
- id = dax_read_lock();
+ dax_read_lock();
switch (pe_size) {
case PE_SIZE_PTE:
fault_size = PAGE_SIZE;
@@ -246,7 +245,7 @@ static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
page->index = pgoff + i;
}
}
- dax_read_unlock(id);
+ dax_read_unlock();
return rc;
}
@@ -284,7 +283,7 @@ static const struct vm_operations_struct dax_vm_ops = {
static int dax_mmap(struct file *filp, struct vm_area_struct *vma)
{
struct dev_dax *dev_dax = filp->private_data;
- int rc, id;
+ int rc;
dev_dbg(_dax->dev, "trace\n");
@@ -292,9 +291,9 @@ static int dax_mmap(struct file *filp, struct
vm_area_struct *vma)
* We lock to check dax_dev liveness and will re-check at
* fault time.
*/
- id = dax_read_lock();
+ dax_read_lock();
rc = check_vma(dev_dax, vma, __func__);
- dax_read_unlock(id);
+ dax_read_unlock();
if (rc)
return rc;
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index fc89e91beea7..48ce86501d93 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -36,7 +36,7 @@ struct dax_device {
};
static dev_t dax_devt;
-DEFINE_STATIC_SRCU(dax_srcu);
+static DECLARE_RWSEM(dax_rwsem);
static struct vfsmount *dax_mnt;
static DEFINE_IDA(dax_minor_ida);
static struct kmem_cache *dax_cache __read_mostly;
@@ -46,18 +46,28 @@ static struct super_block *dax_superblock __read_mostly;
static struct hlist_head dax_host_list[DAX_HASH_SIZE];
static DEFINE_SPINLOCK(dax_host_lock);
-int dax_read_lock(void)
+void dax_read_lock(void)
{
- return srcu_read_lock(_srcu);
+ down_read(_rwsem);
}
EXPORT_SYMBOL_GPL(dax_read_lock);
-void dax_read_unlock(int id)
+void dax_read_unlock(void)
{
- srcu_read_unlock(_srcu, id);
+ up_read(_rwsem);
}
EXPORT_SYMBOL_GPL(dax_read_unlock);
+void dax_write_lock(void)
+{
+ down_write(_rwsem);
+}
+
+void dax_write_unlock(void)
+{
+ up_write(_rwsem);
+}
+
static int dax_host_hash(const char *host)
{
return hashlen_hash(hashlen_string("DAX", host)) % DAX_HASH_SIZE;
@@ -70,14 +80,14 @@ static int dax_host_hash(const char *host)
static struct dax_device *dax_get_by_host(const char *host)
{
struct dax_device *dax_dev, *found = NULL;
- int hash, id;
+ int hash;
if (!host)
return NULL;
hash = dax_host_hash(host);
- id = dax_read_lock();
+ dax_read_lock();
spin_lock(_host_lock);
hlist_for_each_entry(dax_dev, _host_list[hash], list) {
if (!dax_alive(dax_dev)
@@ -89,7 +99,7 @@ static struct dax_device *dax_get_by_host(const char *host)
break;
}
spin_unlock(_host_lock);
- dax_read_unlock(id);
+ dax_read_unlock();
return found;
}
@@ -130,7 +140,7 @@ bool generic_fsdax_supported(struct dax_device *dax_dev,
pfn_t pfn, end_pfn;
sector_t last_page;
long len, len2;
- int err, id;
+ int err;
if (blocksize != PAGE_SIZE) {
pr_info("%pg: error: unsupported blocksize for dax\n", bdev);
@@ -155,14 +165,14 @@ bool generic_fsdax_supported(struct dax_device *dax_dev,
return false;
}
- id = dax_read_lock();
+ dax_read_lock();
len = dax_direct_access(dax_dev, pgoff, 1, , );
len2 = dax_direct_access(dax_dev, pgoff_end, 1, _kaddr, _pfn);
if (len < 1 || len2 < 1) {
pr_info("%pg: error: dax access failed (%ld)\n",
