So in blocklayoutdriver, we can use GFP_KERNEL to do memory
allocation in bl_setup_layoutcommit().
Signed-off-by: Tao Guo <[email protected]>
---
fs/nfs/pnfs.c | 63 ++++++++++++++++++++++++++------------------------------
1 files changed, 29 insertions(+), 34 deletions(-)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index d4e4ba9..bbd1548 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -2075,15 +2075,23 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
* Set up the argument/result storage required for the RPC call.
*/
static int
-pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
+pnfs_layoutcommit_setup(struct inode *inode,
+ struct pnfs_layoutcommit_data *data, int sync)
{
- struct nfs_inode *nfsi = NFS_I(data->args.inode);
- struct nfs_server *nfss = NFS_SERVER(data->args.inode);
+ struct nfs_inode *nfsi = NFS_I(inode);
+ struct nfs_server *nfss = NFS_SERVER(inode);
int result = 0;
dprintk("%s Begin (sync:%d)\n", __func__, sync);
+
+ data->is_sync = sync;
+ data->cred = nfsi->lo_cred;
+ data->args.inode = inode;
data->args.fh = NFS_FH(data->args.inode);
data->args.layout_type = nfss->pnfs_curr_ld->id;
+ pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
+ data->res.fattr = &data->fattr;
+ nfs_fattr_init(&data->fattr);
/* TODO: Need to determine the correct values */
data->args.time_modify_changed = 0;
@@ -2098,19 +2106,21 @@ pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
data->args.bitmask = nfss->attr_bitmask;
data->res.server = nfss;
+ /* Clear layoutcommit properties in the inode so
+ * new lc info can be generated
+ */
+ nfsi->pnfs_write_begin_pos = 0;
+ nfsi->pnfs_write_end_pos = 0;
+ nfsi->lo_cred = NULL;
+
+ spin_unlock(&nfsi->lo_lock);
+
/* Call layout driver to set the arguments.
*/
- if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit) {
+ if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit)
result = nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit(
&nfsi->layout, &data->args);
- if (result)
- goto out;
- }
- pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
- data->res.fattr = &data->fattr;
- nfs_fattr_init(&data->fattr);
-out:
dprintk("%s End Status %d\n", __func__, result);
return result;
}
@@ -2133,39 +2143,24 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
return -ENOMEM;
spin_lock(&nfsi->lo_lock);
- if (!layoutcommit_needed(nfsi))
- goto out_unlock;
-
- data->args.inode = inode;
- data->cred = nfsi->lo_cred;
-
- /* Set up layout commit args*/
- status = pnfs_layoutcommit_setup(data, sync);
-
- /* Clear layoutcommit properties in the inode so
- * new lc info can be generated
- */
- nfsi->pnfs_write_begin_pos = 0;
- nfsi->pnfs_write_end_pos = 0;
- nfsi->lo_cred = NULL;
+ if (!layoutcommit_needed(nfsi)) {
+ spin_unlock(&nfsi->lo_lock);
+ goto out_free;
+ }
+ /* Set up layout commit args */
+ status = pnfs_layoutcommit_setup(inode, data, sync);
if (status) {
/* The layout driver failed to setup the layoutcommit */
put_rpccred(data->cred);
- goto out_unlock;
+ goto out_free;
}
-
- /* release lock on pnfs layoutcommit attrs */
- spin_unlock(&nfsi->lo_lock);
-
- data->is_sync = sync;
status = pnfs4_proc_layoutcommit(data);
out:
dprintk("%s end (err:%d)\n", __func__, status);
return status;
-out_unlock:
+out_free:
pnfs_layoutcommit_free(data);
- spin_unlock(&nfsi->lo_lock);
goto out;
}
--
1.6.3.3
On 05/25/2010 11:51 AM, Tao Guo wrote:
> So in blocklayoutdriver, we can use GFP_KERNEL to do memory
> allocation in bl_setup_layoutcommit().
>
> Signed-off-by: Tao Guo <[email protected]>
> ---
> fs/nfs/pnfs.c | 63 ++++++++++++++++++++++++++------------------------------
> 1 files changed, 29 insertions(+), 34 deletions(-)
>
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index d4e4ba9..bbd1548 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -2075,15 +2075,23 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
> * Set up the argument/result storage required for the RPC call.
> */
> static int
> -pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
> +pnfs_layoutcommit_setup(struct inode *inode,
> + struct pnfs_layoutcommit_data *data, int sync)
> {
> - struct nfs_inode *nfsi = NFS_I(data->args.inode);
> - struct nfs_server *nfss = NFS_SERVER(data->args.inode);
> + struct nfs_inode *nfsi = NFS_I(inode);
> + struct nfs_server *nfss = NFS_SERVER(inode);
> int result = 0;
>
> dprintk("%s Begin (sync:%d)\n", __func__, sync);
> +
> + data->is_sync = sync;
> + data->cred = nfsi->lo_cred;
> + data->args.inode = inode;
> data->args.fh = NFS_FH(data->args.inode);
> data->args.layout_type = nfss->pnfs_curr_ld->id;
> + pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
> + data->res.fattr = &data->fattr;
> + nfs_fattr_init(&data->fattr);
>
> /* TODO: Need to determine the correct values */
> data->args.time_modify_changed = 0;
> @@ -2098,19 +2106,21 @@ pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
> data->args.bitmask = nfss->attr_bitmask;
> data->res.server = nfss;
>
> + /* Clear layoutcommit properties in the inode so
> + * new lc info can be generated
> + */
> + nfsi->pnfs_write_begin_pos = 0;
> + nfsi->pnfs_write_end_pos = 0;
> + nfsi->lo_cred = NULL;
> +
> + spin_unlock(&nfsi->lo_lock);
> +
I don't like it when _unlock is done in a different function then _lock
Please see below proposal for somthing I would think should be more clear.
> /* Call layout driver to set the arguments.
> */
> - if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit) {
> + if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit)
> result = nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit(
> &nfsi->layout, &data->args);
> - if (result)
> - goto out;
> - }
> - pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
> - data->res.fattr = &data->fattr;
> - nfs_fattr_init(&data->fattr);
>
> -out:
> dprintk("%s End Status %d\n", __func__, result);
> return result;
> }
> @@ -2133,39 +2143,24 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
> return -ENOMEM;
>
> spin_lock(&nfsi->lo_lock);
> - if (!layoutcommit_needed(nfsi))
> - goto out_unlock;
> -
> - data->args.inode = inode;
> - data->cred = nfsi->lo_cred;
> -
> - /* Set up layout commit args*/
> - status = pnfs_layoutcommit_setup(data, sync);
> -
> - /* Clear layoutcommit properties in the inode so
> - * new lc info can be generated
> - */
> - nfsi->pnfs_write_begin_pos = 0;
> - nfsi->pnfs_write_end_pos = 0;
> - nfsi->lo_cred = NULL;
> + if (!layoutcommit_needed(nfsi)) {
> + spin_unlock(&nfsi->lo_lock);
> + goto out_free;
> + }
>
> + /* Set up layout commit args */
> + status = pnfs_layoutcommit_setup(inode, data, sync);
> if (status) {
> /* The layout driver failed to setup the layoutcommit */
> put_rpccred(data->cred);
> - goto out_unlock;
> + goto out_free;
> }
> -
> - /* release lock on pnfs layoutcommit attrs */
> - spin_unlock(&nfsi->lo_lock);
> -
> - data->is_sync = sync;
> status = pnfs4_proc_layoutcommit(data);
> out:
> dprintk("%s end (err:%d)\n", __func__, status);
> return status;
> -out_unlock:
> +out_free:
> pnfs_layoutcommit_free(data);
> - spin_unlock(&nfsi->lo_lock);
> goto out;
> }
>
SQUASHME: apply this on top of: pnfs: unlock lo_lock spinlock before calling layoutdriver's setup_layoutcommit
(BTW subject line too long see above)
lock/unlock on same call sight. it is also much more clear what is protected by the spinlock which is
the write_begin/end_pos and most importantly the nfsi->lo_cred which is also the state variable.
(layoutcommit_needed)
I'm also sending a [PATCH -v3] which is a squash of this with Toa's patch.
---
git diff --stat -p -M fs/nfs/pnfs.c
fs/nfs/pnfs.c | 37 ++++++++++++++++++++++---------------
1 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index bbd1548..88d4865 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -2076,7 +2076,8 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
*/
static int
pnfs_layoutcommit_setup(struct inode *inode,
- struct pnfs_layoutcommit_data *data, int sync)
+ struct pnfs_layoutcommit_data *data,
+ loff_t write_begin_pos, loff_t write_end_pos, int sync)
{
struct nfs_inode *nfsi = NFS_I(inode);
struct nfs_server *nfss = NFS_SERVER(inode);
@@ -2099,22 +2100,13 @@ pnfs_layoutcommit_setup(struct inode *inode,
/* Set values from inode so it can be reset
*/
data->args.lseg.iomode = IOMODE_RW;
- data->args.lseg.offset = nfsi->pnfs_write_begin_pos;
- data->args.lseg.length = nfsi->pnfs_write_end_pos - nfsi->pnfs_write_begin_pos + 1;
- data->args.lastbytewritten = min(nfsi->pnfs_write_end_pos,
- i_size_read(&nfsi->vfs_inode) - 1);
+ data->args.lseg.offset = write_begin_pos;
+ data->args.lseg.length = write_end_pos - write_begin_pos + 1;
+ data->args.lastbytewritten = min(write_end_pos,
+ i_size_read(&nfsi->vfs_inode) - 1);
data->args.bitmask = nfss->attr_bitmask;
data->res.server = nfss;
- /* Clear layoutcommit properties in the inode so
- * new lc info can be generated
- */
- nfsi->pnfs_write_begin_pos = 0;
- nfsi->pnfs_write_end_pos = 0;
- nfsi->lo_cred = NULL;
-
- spin_unlock(&nfsi->lo_lock);
-
/* Call layout driver to set the arguments.
*/
if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit)
@@ -2132,6 +2124,9 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
{
struct pnfs_layoutcommit_data *data;
struct nfs_inode *nfsi = NFS_I(inode);
+ loff_t write_begin_pos;
+ loff_t write_end_pos;
+
int status = 0;
dprintk("%s Begin (sync:%d)\n", __func__, sync);
@@ -2148,8 +2143,20 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
goto out_free;
}
+ /* Clear layoutcommit properties in the inode so
+ * new lc info can be generated
+ */
+ write_begin_pos = nfsi->pnfs_write_begin_pos;
+ write_end_pos = nfsi->pnfs_write_end_pos;
+ nfsi->pnfs_write_begin_pos = 0;
+ nfsi->pnfs_write_end_pos = 0;
+ nfsi->lo_cred = NULL;
+
+ spin_unlock(&nfsi->lo_lock);
+
/* Set up layout commit args */
- status = pnfs_layoutcommit_setup(inode, data, sync);
+ status = pnfs_layoutcommit_setup(inode, data, write_begin_pos,
+ write_end_pos, sync);
if (status) {
/* The layout driver failed to setup the layoutcommit */
put_rpccred(data->cred);
On 05/25/2010 04:01 PM, Boaz Harrosh wrote:
> On 05/25/2010 11:51 AM, Tao Guo wrote:
>> So in blocklayoutdriver, we can use GFP_KERNEL to do memory
>> allocation in bl_setup_layoutcommit().
>>
>> Signed-off-by: Tao Guo <[email protected]>
>> ---
>> fs/nfs/pnfs.c | 63 ++++++++++++++++++++++++++------------------------------
>> 1 files changed, 29 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index d4e4ba9..bbd1548 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -2075,15 +2075,23 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
>> * Set up the argument/result storage required for the RPC call.
>> */
>> static int
>> -pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
>> +pnfs_layoutcommit_setup(struct inode *inode,
>> + struct pnfs_layoutcommit_data *data, int sync)
>> {
>> - struct nfs_inode *nfsi = NFS_I(data->args.inode);
>> - struct nfs_server *nfss = NFS_SERVER(data->args.inode);
>> + struct nfs_inode *nfsi = NFS_I(inode);
>> + struct nfs_server *nfss = NFS_SERVER(inode);
>> int result = 0;
>>
>> dprintk("%s Begin (sync:%d)\n", __func__, sync);
>> +
>> + data->is_sync = sync;
>> + data->cred = nfsi->lo_cred;
>> + data->args.inode = inode;
>> data->args.fh = NFS_FH(data->args.inode);
>> data->args.layout_type = nfss->pnfs_curr_ld->id;
>> + pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
>> + data->res.fattr = &data->fattr;
>> + nfs_fattr_init(&data->fattr);
>>
>> /* TODO: Need to determine the correct values */
>> data->args.time_modify_changed = 0;
>> @@ -2098,19 +2106,21 @@ pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
>> data->args.bitmask = nfss->attr_bitmask;
>> data->res.server = nfss;
>>
>> + /* Clear layoutcommit properties in the inode so
>> + * new lc info can be generated
>> + */
>> + nfsi->pnfs_write_begin_pos = 0;
>> + nfsi->pnfs_write_end_pos = 0;
>> + nfsi->lo_cred = NULL;
>> +
>> + spin_unlock(&nfsi->lo_lock);
>> +
>
> I don't like it when _unlock is done in a different function then _lock
>
> Please see below proposal for somthing I would think should be more clear.
>
>> /* Call layout driver to set the arguments.
>> */
>> - if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit) {
>> + if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit)
>> result = nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit(
>> &nfsi->layout, &data->args);
>> - if (result)
>> - goto out;
>> - }
>> - pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
>> - data->res.fattr = &data->fattr;
>> - nfs_fattr_init(&data->fattr);
>>
>> -out:
>> dprintk("%s End Status %d\n", __func__, result);
>> return result;
>> }
>> @@ -2133,39 +2143,24 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
>> return -ENOMEM;
>>
>> spin_lock(&nfsi->lo_lock);
>> - if (!layoutcommit_needed(nfsi))
>> - goto out_unlock;
>> -
>> - data->args.inode = inode;
>> - data->cred = nfsi->lo_cred;
>> -
>> - /* Set up layout commit args*/
>> - status = pnfs_layoutcommit_setup(data, sync);
>> -
>> - /* Clear layoutcommit properties in the inode so
>> - * new lc info can be generated
>> - */
>> - nfsi->pnfs_write_begin_pos = 0;
>> - nfsi->pnfs_write_end_pos = 0;
>> - nfsi->lo_cred = NULL;
>> + if (!layoutcommit_needed(nfsi)) {
>> + spin_unlock(&nfsi->lo_lock);
>> + goto out_free;
>> + }
>>
>> + /* Set up layout commit args */
>> + status = pnfs_layoutcommit_setup(inode, data, sync);
>> if (status) {
>> /* The layout driver failed to setup the layoutcommit */
>> put_rpccred(data->cred);
>> - goto out_unlock;
>> + goto out_free;
>> }
>> -
>> - /* release lock on pnfs layoutcommit attrs */
>> - spin_unlock(&nfsi->lo_lock);
>> -
>> - data->is_sync = sync;
>> status = pnfs4_proc_layoutcommit(data);
>> out:
>> dprintk("%s end (err:%d)\n", __func__, status);
>> return status;
>> -out_unlock:
>> +out_free:
>> pnfs_layoutcommit_free(data);
>> - spin_unlock(&nfsi->lo_lock);
>> goto out;
>> }
>>
>
> SQUASHME: apply this on top of: pnfs: unlock lo_lock spinlock before calling layoutdriver's setup_layoutcommit
>
> (BTW subject line too long see above)
>
> lock/unlock on same call sight. it is also much more clear what is protected by the spinlock which is
> the write_begin/end_pos and most importantly the nfsi->lo_cred which is also the state variable.
> (layoutcommit_needed)
>
> I'm also sending a [PATCH -v3] which is a squash of this with Toa's patch.
>
> ---
> git diff --stat -p -M fs/nfs/pnfs.c
> fs/nfs/pnfs.c | 37 ++++++++++++++++++++++---------------
> 1 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index bbd1548..88d4865 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -2076,7 +2076,8 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
> */
> static int
> pnfs_layoutcommit_setup(struct inode *inode,
> - struct pnfs_layoutcommit_data *data, int sync)
> + struct pnfs_layoutcommit_data *data,
> + loff_t write_begin_pos, loff_t write_end_pos, int sync)
> {
> struct nfs_inode *nfsi = NFS_I(inode);
> struct nfs_server *nfss = NFS_SERVER(inode);
> @@ -2099,22 +2100,13 @@ pnfs_layoutcommit_setup(struct inode *inode,
> /* Set values from inode so it can be reset
> */
> data->args.lseg.iomode = IOMODE_RW;
> - data->args.lseg.offset = nfsi->pnfs_write_begin_pos;
> - data->args.lseg.length = nfsi->pnfs_write_end_pos - nfsi->pnfs_write_begin_pos + 1;
> - data->args.lastbytewritten = min(nfsi->pnfs_write_end_pos,
> - i_size_read(&nfsi->vfs_inode) - 1);
> + data->args.lseg.offset = write_begin_pos;
> + data->args.lseg.length = write_end_pos - write_begin_pos + 1;
> + data->args.lastbytewritten = min(write_end_pos,
> + i_size_read(&nfsi->vfs_inode) - 1);
> data->args.bitmask = nfss->attr_bitmask;
> data->res.server = nfss;
>
> - /* Clear layoutcommit properties in the inode so
> - * new lc info can be generated
> - */
> - nfsi->pnfs_write_begin_pos = 0;
> - nfsi->pnfs_write_end_pos = 0;
> - nfsi->lo_cred = NULL;
> -
> - spin_unlock(&nfsi->lo_lock);
> -
> /* Call layout driver to set the arguments.
> */
> if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit)
> @@ -2132,6 +2124,9 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
> {
> struct pnfs_layoutcommit_data *data;
> struct nfs_inode *nfsi = NFS_I(inode);
> + loff_t write_begin_pos;
> + loff_t write_end_pos;
> +
> int status = 0;
>
> dprintk("%s Begin (sync:%d)\n", __func__, sync);
> @@ -2148,8 +2143,20 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
> goto out_free;
> }
>
> + /* Clear layoutcommit properties in the inode so
> + * new lc info can be generated
> + */
> + write_begin_pos = nfsi->pnfs_write_begin_pos;
> + write_end_pos = nfsi->pnfs_write_end_pos;
> + nfsi->pnfs_write_begin_pos = 0;
> + nfsi->pnfs_write_end_pos = 0;
> + nfsi->lo_cred = NULL;
> +
> + spin_unlock(&nfsi->lo_lock);
> +
> /* Set up layout commit args */
> - status = pnfs_layoutcommit_setup(inode, data, sync);
> + status = pnfs_layoutcommit_setup(inode, data, write_begin_pos,
> + write_end_pos, sync);
> if (status) {
> /* The layout driver failed to setup the layoutcommit */
> put_rpccred(data->cred);
>
>
So that had a bug, should test before posting. Here is a 3rd squashme
on top of my squashme. (Am sending a v3 yet)
Also probably pnfs_get_layout_stateid should be under the lock, so
fix that too.
Boaz
---
git diff --stat -p -M fs/nfs/pnfs.c
fs/nfs/pnfs.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 88d4865..cf9cfe5 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -2086,11 +2086,9 @@ pnfs_layoutcommit_setup(struct inode *inode,
dprintk("%s Begin (sync:%d)\n", __func__, sync);
data->is_sync = sync;
- data->cred = nfsi->lo_cred;
data->args.inode = inode;
- data->args.fh = NFS_FH(data->args.inode);
+ data->args.fh = NFS_FH(inode);
data->args.layout_type = nfss->pnfs_curr_ld->id;
- pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
data->res.fattr = &data->fattr;
nfs_fattr_init(&data->fattr);
@@ -2103,7 +2101,7 @@ pnfs_layoutcommit_setup(struct inode *inode,
data->args.lseg.offset = write_begin_pos;
data->args.lseg.length = write_end_pos - write_begin_pos + 1;
data->args.lastbytewritten = min(write_end_pos,
- i_size_read(&nfsi->vfs_inode) - 1);
+ i_size_read(inode) - 1);
data->args.bitmask = nfss->attr_bitmask;
data->res.server = nfss;
@@ -2148,9 +2146,11 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
*/
write_begin_pos = nfsi->pnfs_write_begin_pos;
write_end_pos = nfsi->pnfs_write_end_pos;
+ data->cred = nfsi->lo_cred;
nfsi->pnfs_write_begin_pos = 0;
nfsi->pnfs_write_end_pos = 0;
nfsi->lo_cred = NULL;
+ pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
spin_unlock(&nfsi->lo_lock);
From: Tao Guo <[email protected]>
So in blocklayoutdriver, we can use GFP_KERNEL to do memory
allocation in bl_setup_layoutcommit().
The state protected by the lo_lock here is clear now, which is the
layout_commit's position and state. .i.e write_begin/end_pos,
nfsi->lo_cred, and the call to pnfs_get_layout_stateid.
Signed-off-by: Tao Guo <[email protected]>
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/nfs/pnfs.c | 66 +++++++++++++++++++++++++++++---------------------------
1 files changed, 34 insertions(+), 32 deletions(-)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index d4e4ba9..cf9cfe5 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -2075,15 +2075,22 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
* Set up the argument/result storage required for the RPC call.
*/
static int
-pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
+pnfs_layoutcommit_setup(struct inode *inode,
+ struct pnfs_layoutcommit_data *data,
+ loff_t write_begin_pos, loff_t write_end_pos, int sync)
{
- struct nfs_inode *nfsi = NFS_I(data->args.inode);
- struct nfs_server *nfss = NFS_SERVER(data->args.inode);
+ struct nfs_inode *nfsi = NFS_I(inode);
+ struct nfs_server *nfss = NFS_SERVER(inode);
int result = 0;
dprintk("%s Begin (sync:%d)\n", __func__, sync);
- data->args.fh = NFS_FH(data->args.inode);
+
+ data->is_sync = sync;
+ data->args.inode = inode;
+ data->args.fh = NFS_FH(inode);
data->args.layout_type = nfss->pnfs_curr_ld->id;
+ data->res.fattr = &data->fattr;
+ nfs_fattr_init(&data->fattr);
/* TODO: Need to determine the correct values */
data->args.time_modify_changed = 0;
@@ -2091,26 +2098,19 @@ pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
/* Set values from inode so it can be reset
*/
data->args.lseg.iomode = IOMODE_RW;
- data->args.lseg.offset = nfsi->pnfs_write_begin_pos;
- data->args.lseg.length = nfsi->pnfs_write_end_pos - nfsi->pnfs_write_begin_pos + 1;
- data->args.lastbytewritten = min(nfsi->pnfs_write_end_pos,
- i_size_read(&nfsi->vfs_inode) - 1);
+ data->args.lseg.offset = write_begin_pos;
+ data->args.lseg.length = write_end_pos - write_begin_pos + 1;
+ data->args.lastbytewritten = min(write_end_pos,
+ i_size_read(inode) - 1);
data->args.bitmask = nfss->attr_bitmask;
data->res.server = nfss;
/* Call layout driver to set the arguments.
*/
- if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit) {
+ if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit)
result = nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit(
&nfsi->layout, &data->args);
- if (result)
- goto out;
- }
- pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
- data->res.fattr = &data->fattr;
- nfs_fattr_init(&data->fattr);
-out:
dprintk("%s End Status %d\n", __func__, result);
return result;
}
@@ -2122,6 +2122,9 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
{
struct pnfs_layoutcommit_data *data;
struct nfs_inode *nfsi = NFS_I(inode);
+ loff_t write_begin_pos;
+ loff_t write_end_pos;
+
int status = 0;
dprintk("%s Begin (sync:%d)\n", __func__, sync);
@@ -2133,39 +2136,38 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
return -ENOMEM;
spin_lock(&nfsi->lo_lock);
- if (!layoutcommit_needed(nfsi))
- goto out_unlock;
-
- data->args.inode = inode;
- data->cred = nfsi->lo_cred;
-
- /* Set up layout commit args*/
- status = pnfs_layoutcommit_setup(data, sync);
+ if (!layoutcommit_needed(nfsi)) {
+ spin_unlock(&nfsi->lo_lock);
+ goto out_free;
+ }
/* Clear layoutcommit properties in the inode so
* new lc info can be generated
*/
+ write_begin_pos = nfsi->pnfs_write_begin_pos;
+ write_end_pos = nfsi->pnfs_write_end_pos;
+ data->cred = nfsi->lo_cred;
nfsi->pnfs_write_begin_pos = 0;
nfsi->pnfs_write_end_pos = 0;
nfsi->lo_cred = NULL;
+ pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
+ spin_unlock(&nfsi->lo_lock);
+
+ /* Set up layout commit args */
+ status = pnfs_layoutcommit_setup(inode, data, write_begin_pos,
+ write_end_pos, sync);
if (status) {
/* The layout driver failed to setup the layoutcommit */
put_rpccred(data->cred);
- goto out_unlock;
+ goto out_free;
}
-
- /* release lock on pnfs layoutcommit attrs */
- spin_unlock(&nfsi->lo_lock);
-
- data->is_sync = sync;
status = pnfs4_proc_layoutcommit(data);
out:
dprintk("%s end (err:%d)\n", __func__, status);
return status;
-out_unlock:
+out_free:
pnfs_layoutcommit_free(data);
- spin_unlock(&nfsi->lo_lock);
goto out;
}
--
1.6.6.1
Boaz Harrosh wrote:
> From: Tao Guo <[email protected]>
>
> So in blocklayoutdriver, we can use GFP_KERNEL to do memory
> allocation in bl_setup_layoutcommit().
>
> The state protected by the lo_lock here is clear now, which is the
> layout_commit's position and state. .i.e write_begin/end_pos,
> nfsi->lo_cred, and the call to pnfs_get_layout_stateid.
Yeah, this looks cleaner.
Tao, can you please test and ack this patch before I merge it?
Thanks!
Benny
>
> Signed-off-by: Tao Guo <[email protected]>
> Signed-off-by: Boaz Harrosh <[email protected]>
> ---
> fs/nfs/pnfs.c | 66 +++++++++++++++++++++++++++++---------------------------
> 1 files changed, 34 insertions(+), 32 deletions(-)
>
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index d4e4ba9..cf9cfe5 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -2075,15 +2075,22 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
> * Set up the argument/result storage required for the RPC call.
> */
> static int
> -pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
> +pnfs_layoutcommit_setup(struct inode *inode,
> + struct pnfs_layoutcommit_data *data,
> + loff_t write_begin_pos, loff_t write_end_pos, int sync)
> {
> - struct nfs_inode *nfsi = NFS_I(data->args.inode);
> - struct nfs_server *nfss = NFS_SERVER(data->args.inode);
> + struct nfs_inode *nfsi = NFS_I(inode);
> + struct nfs_server *nfss = NFS_SERVER(inode);
> int result = 0;
>
> dprintk("%s Begin (sync:%d)\n", __func__, sync);
> - data->args.fh = NFS_FH(data->args.inode);
> +
> + data->is_sync = sync;
> + data->args.inode = inode;
> + data->args.fh = NFS_FH(inode);
> data->args.layout_type = nfss->pnfs_curr_ld->id;
> + data->res.fattr = &data->fattr;
> + nfs_fattr_init(&data->fattr);
>
> /* TODO: Need to determine the correct values */
> data->args.time_modify_changed = 0;
> @@ -2091,26 +2098,19 @@ pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
> /* Set values from inode so it can be reset
> */
> data->args.lseg.iomode = IOMODE_RW;
> - data->args.lseg.offset = nfsi->pnfs_write_begin_pos;
> - data->args.lseg.length = nfsi->pnfs_write_end_pos - nfsi->pnfs_write_begin_pos + 1;
> - data->args.lastbytewritten = min(nfsi->pnfs_write_end_pos,
> - i_size_read(&nfsi->vfs_inode) - 1);
> + data->args.lseg.offset = write_begin_pos;
> + data->args.lseg.length = write_end_pos - write_begin_pos + 1;
> + data->args.lastbytewritten = min(write_end_pos,
> + i_size_read(inode) - 1);
> data->args.bitmask = nfss->attr_bitmask;
> data->res.server = nfss;
>
> /* Call layout driver to set the arguments.
> */
> - if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit) {
> + if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit)
> result = nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit(
> &nfsi->layout, &data->args);
> - if (result)
> - goto out;
> - }
> - pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
> - data->res.fattr = &data->fattr;
> - nfs_fattr_init(&data->fattr);
>
> -out:
> dprintk("%s End Status %d\n", __func__, result);
> return result;
> }
> @@ -2122,6 +2122,9 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
> {
> struct pnfs_layoutcommit_data *data;
> struct nfs_inode *nfsi = NFS_I(inode);
> + loff_t write_begin_pos;
> + loff_t write_end_pos;
> +
> int status = 0;
>
> dprintk("%s Begin (sync:%d)\n", __func__, sync);
> @@ -2133,39 +2136,38 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
> return -ENOMEM;
>
> spin_lock(&nfsi->lo_lock);
> - if (!layoutcommit_needed(nfsi))
> - goto out_unlock;
> -
> - data->args.inode = inode;
> - data->cred = nfsi->lo_cred;
> -
> - /* Set up layout commit args*/
> - status = pnfs_layoutcommit_setup(data, sync);
> + if (!layoutcommit_needed(nfsi)) {
> + spin_unlock(&nfsi->lo_lock);
> + goto out_free;
> + }
>
> /* Clear layoutcommit properties in the inode so
> * new lc info can be generated
> */
> + write_begin_pos = nfsi->pnfs_write_begin_pos;
> + write_end_pos = nfsi->pnfs_write_end_pos;
> + data->cred = nfsi->lo_cred;
> nfsi->pnfs_write_begin_pos = 0;
> nfsi->pnfs_write_end_pos = 0;
> nfsi->lo_cred = NULL;
> + pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
>
> + spin_unlock(&nfsi->lo_lock);
> +
> + /* Set up layout commit args */
> + status = pnfs_layoutcommit_setup(inode, data, write_begin_pos,
> + write_end_pos, sync);
> if (status) {
> /* The layout driver failed to setup the layoutcommit */
> put_rpccred(data->cred);
> - goto out_unlock;
> + goto out_free;
> }
> -
> - /* release lock on pnfs layoutcommit attrs */
> - spin_unlock(&nfsi->lo_lock);
> -
> - data->is_sync = sync;
> status = pnfs4_proc_layoutcommit(data);
> out:
> dprintk("%s end (err:%d)\n", __func__, status);
> return status;
> -out_unlock:
> +out_free:
> pnfs_layoutcommit_free(data);
> - spin_unlock(&nfsi->lo_lock);
> goto out;
> }
>
On Tue, May 25, 2010 at 10:40 PM, Benny Halevy <[email protected]> wrote:
> Boaz Harrosh wrote:
>> From: Tao Guo <[email protected]>
>>
>> So in blocklayoutdriver, we can use GFP_KERNEL to do memory
>> allocation in bl_setup_layoutcommit().
>>
>> The state protected by the lo_lock here is clear now, which is the
>> layout_commit's position and state. .i.e write_begin/end_pos,
>> nfsi->lo_cred, and the call to pnfs_get_layout_stateid.
>
> Yeah, this looks cleaner.
Yes, thanks.
> Tao, can you please test and ack this patch before I merge it?
>
> Thanks!
>
> Benny
>
Ok, I will test it tomorrow morning.
--
Tao.
On Tue, May 25, 2010 at 11:26 PM, Tao Guo <[email protected]> wrote:
> On Tue, May 25, 2010 at 10:40 PM, Benny Halevy <[email protected]> wrote:
>> Boaz Harrosh wrote:
>>> From: Tao Guo <[email protected]>
>>>
>>> So in blocklayoutdriver, we can use GFP_KERNEL to do memory
>>> allocation in bl_setup_layoutcommit().
>>>
>>> The state protected by the lo_lock here is clear now, which is the
>>> layout_commit's position and state. .i.e write_begin/end_pos,
>>> nfsi->lo_cred, and the call to pnfs_get_layout_stateid.
>>
>> Yeah, this looks cleaner.
> Yes, thanks.
>> Tao, can you please test and ack this patch before I merge it?
>>
>> Thanks!
>>
>> Benny
I have tested it, it is OK.
Tao
>