2015-11-07 07:41:32

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 0/9] Remove wrapper function and tidy up the code

This patchset removes an unnecessary wrapper function and replaces all
its calls in different files with the standard function that it wrapped.
Also, one patch removes its prototype.
After applying this patch, code becomes cleaner.

Shivani Bhardwaj (9):
Staging: lustre: dir: Replace function calls
Staging: lustre: file: Replace function calls with standard function
Staging: lustre: namei: Replace calls with kfree
Staging: lustre: xattr_cache: Change function calls
Staging: lustre: symlink: Substitute standard function
Staging: lustre: llite_nfs: Replace with standard function
Staging: lustre: llite_close: Substitute function calls
Staging: lustre: llite_lib: Remove wrapper function
Staging: lustre: llite_internal: Remove function prototype

drivers/staging/lustre/lustre/llite/dir.c | 14 ++++++-------
drivers/staging/lustre/lustre/llite/file.c | 24 +++++++++++-----------
drivers/staging/lustre/lustre/llite/llite_close.c | 2 +-
.../staging/lustre/lustre/llite/llite_internal.h | 1 -
drivers/staging/lustre/lustre/llite/llite_lib.c | 13 ++++--------
drivers/staging/lustre/lustre/llite/llite_nfs.c | 2 +-
drivers/staging/lustre/lustre/llite/namei.c | 12 +++++------
drivers/staging/lustre/lustre/llite/symlink.c | 2 +-
drivers/staging/lustre/lustre/llite/xattr_cache.c | 2 +-
9 files changed, 33 insertions(+), 39 deletions(-)

--
2.1.0


2015-11-07 07:42:26

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 1/9] Staging: lustre: dir: Replace function calls

Replace the calls of the function ll_finish_md_op_data() with the
standard function kfree().

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/dir.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index 5c2ef92..ba3f469 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -182,7 +182,7 @@ static int ll_dir_filler(void *_hash, struct page *page0)
op_data->op_npages = npages;
op_data->op_offset = hash;
rc = md_readpage(exp, op_data, page_pool, &request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc < 0) {
/* page0 is special, which was added into page cache early */
delete_from_page_cache(page0);
@@ -363,7 +363,7 @@ struct page *ll_get_dir_page(struct inode *dir, __u64 hash,
rc = md_enqueue(ll_i2sbi(dir)->ll_md_exp, &einfo, &it,
op_data, &lockh, NULL, 0, NULL, 0);

- ll_finish_md_op_data(op_data);
+ kfree(op_data);

request = (struct ptlrpc_request *)it.d.lustre.it_data;
if (request)
@@ -669,7 +669,7 @@ static int ll_dir_setdirstripe(struct inode *dir, struct lmv_user_md *lump,
from_kuid(&init_user_ns, current_fsuid()),
from_kgid(&init_user_ns, current_fsgid()),
cfs_curproc_cap_pack(), 0, &request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (err)
goto err_exit;
err_exit:
@@ -730,7 +730,7 @@ int ll_dir_setstripe(struct inode *inode, struct lov_user_md *lump,
/* swabbing is done in lov_setstripe() on server side */
rc = md_setattr(sbi->ll_md_exp, op_data, lump, lum_size,
NULL, 0, &req, NULL);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
ptlrpc_req_finished(req);
if (rc) {
if (rc != -EPERM && rc != -EACCES)
@@ -802,7 +802,7 @@ int ll_dir_getstripe(struct inode *inode, struct lov_mds_md **lmmp,

op_data->op_valid = OBD_MD_FLEASIZE | OBD_MD_FLDIREA;
rc = md_getattr(sbi->ll_md_exp, op_data, &req);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc < 0) {
CDEBUG(D_INFO, "md_getattr failed on inode %lu/%u: rc %d\n",
inode->i_ino,
@@ -868,7 +868,7 @@ int ll_get_mdt_idx(struct inode *inode)
op_data->op_flags |= MF_GET_MDT_IDX;
rc = md_getattr(sbi->ll_md_exp, op_data, NULL);
mdtidx = op_data->op_mds;
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc < 0) {
CDEBUG(D_INFO, "md_getattr_name: %d\n", rc);
return rc;
@@ -1301,7 +1301,7 @@ static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)

op_data->op_valid = OBD_MD_FLID;
rc = md_getattr_name(sbi->ll_md_exp, op_data, &request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc < 0) {
CDEBUG(D_INFO, "md_getattr_name: %d\n", rc);
goto out_free;
--
2.1.0

2015-11-07 07:43:14

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 2/9] Staging: lustre: file: Replace function calls with standard function

Replace the calls of the function ll_finish_md_op_data() with the
standard function kfree().

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/file.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index 02f2759..186b5af 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -202,7 +202,7 @@ static int ll_close_inode_openhandle(struct obd_export *md_exp,
rc = -EBUSY;
}

- ll_finish_md_op_data(op_data);
+ kfree(op_data);

out:
if (exp_connect_som(exp) && !epoch_close &&
@@ -420,7 +420,7 @@ static int ll_intent_file_open(struct dentry *dentry, void *lmm,
itp->it_flags |= MDS_OPEN_BY_FID;
rc = md_intent_lock(sbi->ll_md_exp, op_data, lmm, lmmsize, itp,
0 /*unused */, &req, ll_md_blocking_ast, 0);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc == -ESTALE) {
/* reason for keep own exit path - don`t flood log
* with messages with -ESTALE errors.
@@ -819,7 +819,7 @@ ll_lease_open(struct inode *inode, struct file *file, fmode_t fmode,
* open in ll_md_blocking_ast(). Otherwise as ll_md_blocking_lease_ast
* doesn't deal with openhandle, so normal openhandle will be leaked. */
LDLM_FL_NO_LRU | LDLM_FL_EXCL);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
ptlrpc_req_finished(req);
if (rc < 0)
goto out_release_it;
@@ -1393,7 +1393,7 @@ int ll_lov_getstripe_ea_info(struct inode *inode, const char *filename,

op_data->op_valid = OBD_MD_FLEASIZE | OBD_MD_FLDIREA;
rc = md_getattr_name(sbi->ll_md_exp, op_data, &req);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc < 0) {
CDEBUG(D_INFO, "md_getattr_name failed on %s: rc %d\n",
filename, rc);
@@ -2056,7 +2056,7 @@ static int ll_swap_layouts(struct file *file1, struct file *file2,

rc = obd_iocontrol(LL_IOC_LOV_SWAP_LAYOUTS, ll_i2mdexp(llss->inode1),
sizeof(*op_data), op_data, NULL);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);

putgl:
if (gid != 0) {
@@ -2131,7 +2131,7 @@ static int ll_hsm_state_set(struct inode *inode, struct hsm_state_set *hss)
rc = obd_iocontrol(LL_IOC_HSM_STATE_SET, ll_i2mdexp(inode),
sizeof(*op_data), op_data, NULL);

- ll_finish_md_op_data(op_data);
+ kfree(op_data);

return rc;
}
@@ -2350,7 +2350,7 @@ ll_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (copy_to_user((void *)arg, hus, sizeof(*hus)))
rc = -EFAULT;

- ll_finish_md_op_data(op_data);
+ kfree(op_data);
kfree(hus);
return rc;
}
@@ -2389,7 +2389,7 @@ ll_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (copy_to_user((char *)arg, hca, sizeof(*hca)))
rc = -EFAULT;

- ll_finish_md_op_data(op_data);
+ kfree(op_data);
kfree(hca);
return rc;
}
@@ -2761,7 +2761,7 @@ ll_file_flock(struct file *file, int cmd, struct file_lock *file_lock)
rc = rc2;
}

- ll_finish_md_op_data(op_data);
+ kfree(op_data);

return rc;
}
@@ -2896,7 +2896,7 @@ static int __ll_inode_revalidate(struct dentry *dentry, __u64 ibits)
based lookup */
&oit, 0, &req,
ll_md_blocking_ast, 0);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
oit.it_create_mode &= ~M_CHECK_STALE;
if (rc < 0) {
rc = ll_inode_revalidate_fini(inode, rc);
@@ -2938,7 +2938,7 @@ static int __ll_inode_revalidate(struct dentry *dentry, __u64 ibits)

op_data->op_valid = valid;
rc = md_getattr(sbi->ll_md_exp, op_data, &req);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc) {
rc = ll_inode_revalidate_fini(inode, rc);
return rc;
@@ -3533,7 +3533,7 @@ again:
ptlrpc_req_finished(it.d.lustre.it_data);
it.d.lustre.it_data = NULL;

- ll_finish_md_op_data(op_data);
+ kfree(op_data);

mode = it.d.lustre.it_lock_mode;
it.d.lustre.it_lock_mode = 0;
--
2.1.0

2015-11-07 07:44:13

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 3/9] Staging: lustre: namei: Replace calls with kfree

Replace the calls of the function ll_finish_md_op_data() with thr
stndard function kfree().

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/namei.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c
index 2ca2200..f058ebe2 100644
--- a/drivers/staging/lustre/lustre/llite/namei.c
+++ b/drivers/staging/lustre/lustre/llite/namei.c
@@ -531,7 +531,7 @@ static struct dentry *ll_lookup_it(struct inode *parent, struct dentry *dentry,

rc = md_intent_lock(ll_i2mdexp(parent), op_data, NULL, 0, it,
lookup_flags, &req, ll_md_blocking_ast, 0);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc < 0) {
retval = ERR_PTR(rc);
goto out;
@@ -786,7 +786,7 @@ static int ll_new_node(struct inode *dir, struct dentry *dentry,
from_kuid(&init_user_ns, current_fsuid()),
from_kgid(&init_user_ns, current_fsgid()),
cfs_curproc_cap_pack(), rdev, &request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (err)
goto err_exit;

@@ -961,7 +961,7 @@ static int ll_unlink(struct inode *dir, struct dentry *dentry)
ll_get_child_fid(dentry, &op_data->op_fid3);
op_data->op_fid2 = op_data->op_fid3;
rc = md_unlink(ll_i2sbi(dir)->ll_md_exp, op_data, &request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc)
goto out;

@@ -1011,7 +1011,7 @@ static int ll_rmdir(struct inode *dir, struct dentry *dentry)
ll_get_child_fid(dentry, &op_data->op_fid3);
op_data->op_fid2 = op_data->op_fid3;
rc = md_unlink(ll_i2sbi(dir)->ll_md_exp, op_data, &request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc == 0) {
ll_update_times(request, dir);
ll_stats_ops_tally(ll_i2sbi(dir), LPROC_LL_RMDIR, 1);
@@ -1060,7 +1060,7 @@ static int ll_link(struct dentry *old_dentry, struct inode *dir,
return PTR_ERR(op_data);

err = md_link(sbi->ll_md_exp, op_data, &request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (err)
goto out;

@@ -1096,7 +1096,7 @@ static int ll_rename(struct inode *old_dir, struct dentry *old_dentry,
old_dentry->d_name.len,
new_dentry->d_name.name,
new_dentry->d_name.len, &request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (!err) {
ll_update_times(request, old_dir);
ll_update_times(request, new_dir);
--
2.1.0

2015-11-07 07:44:55

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 4/9] Staging: lustre: xattr_cache: Change function calls

Change the calls of the function ll_finish_md_op_data() to the standard
function kfree().

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/xattr_cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/xattr_cache.c b/drivers/staging/lustre/lustre/llite/xattr_cache.c
index e1e599c..b7f3efc 100644
--- a/drivers/staging/lustre/lustre/llite/xattr_cache.c
+++ b/drivers/staging/lustre/lustre/llite/xattr_cache.c
@@ -306,7 +306,7 @@ static int ll_xattr_find_get_lock(struct inode *inode,
op_data->op_valid = OBD_MD_FLXATTR | OBD_MD_FLXATTRLS;

rc = md_enqueue(exp, &einfo, oit, op_data, &lockh, NULL, 0, NULL, 0);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);

if (rc < 0) {
CDEBUG(D_CACHE,
--
2.1.0

2015-11-07 07:45:30

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 5/9] Staging: lustre: symlink: Substitute standard function

Substitute the standard function kfree() for the function
ll_finish_md_op_data().

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/symlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/symlink.c b/drivers/staging/lustre/lustre/llite/symlink.c
index 69b2036..6b0d1cc 100644
--- a/drivers/staging/lustre/lustre/llite/symlink.c
+++ b/drivers/staging/lustre/lustre/llite/symlink.c
@@ -73,7 +73,7 @@ static int ll_readlink_internal(struct inode *inode,

op_data->op_valid = OBD_MD_LINKNAME;
rc = md_getattr(sbi->ll_md_exp, op_data, request);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc) {
if (rc != -ENOENT)
CERROR("inode %lu: rc = %d\n", inode->i_ino, rc);
--
2.1.0

2015-11-07 07:46:09

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 6/9] Staging: lustre: llite_nfs: Replace with standard function

Replace the calls of the function ll_finish_md_op_data() with the
standard function kfree().

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/llite_nfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_nfs.c b/drivers/staging/lustre/lustre/llite/llite_nfs.c
index e578a11..a3a89a7 100644
--- a/drivers/staging/lustre/lustre/llite/llite_nfs.c
+++ b/drivers/staging/lustre/lustre/llite/llite_nfs.c
@@ -306,7 +306,7 @@ static struct dentry *ll_get_parent(struct dentry *dchild)
return (void *)op_data;

rc = md_getattr_name(sbi->ll_md_exp, op_data, &req);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc) {
CERROR("failure %d inode %lu get parent\n", rc, dir->i_ino);
return ERR_PTR(rc);
--
2.1.0

2015-11-07 07:46:43

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 7/9] Staging: lustre: llite_close: Substitute function calls

Substitute standard function kfree() in place of the function
ll_finish_md_op_data().

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/llite_close.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_close.c b/drivers/staging/lustre/lustre/llite/llite_close.c
index 3f348a3..c4f22ec 100644
--- a/drivers/staging/lustre/lustre/llite/llite_close.c
+++ b/drivers/staging/lustre/lustre/llite/llite_close.c
@@ -301,7 +301,7 @@ static void ll_done_writing(struct inode *inode)
CERROR("inode %lu mdc done_writing failed: rc = %d\n",
inode->i_ino, rc);
out:
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (och) {
md_clear_open_replay_data(ll_i2sbi(inode)->ll_md_exp, och);
kfree(och);
--
2.1.0

2015-11-07 07:47:20

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 8/9] Staging: lustre: llite_lib: Remove wrapper function

Remove the function ll_finish_md_op_data() and replace all its calls
with the standrd function ll_finish_md_op_data().

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/llite_lib.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
index 4a8c759..143be87 100644
--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
@@ -1355,7 +1355,7 @@ out:
if (!rc)
rc = rc1;
}
- ll_finish_md_op_data(op_data);
+ kfree(op_data);

if (!S_ISDIR(inode->i_mode)) {
mutex_lock(&inode->i_mutex);
@@ -1732,7 +1732,7 @@ int ll_iocontrol(struct inode *inode, struct file *file,

op_data->op_valid = OBD_MD_FLFLAGS;
rc = md_getattr(sbi->ll_md_exp, op_data, &req);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
if (rc) {
CERROR("failure %d inode %lu\n", rc, inode->i_ino);
return -abs(rc);
@@ -1763,7 +1763,7 @@ int ll_iocontrol(struct inode *inode, struct file *file,
op_data->op_attr.ia_valid |= ATTR_ATTR_FLAG;
rc = md_setattr(sbi->ll_md_exp, op_data,
NULL, 0, NULL, 0, &req, NULL);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
ptlrpc_req_finished(req);
if (rc)
return rc;
@@ -1934,7 +1934,7 @@ void ll_open_cleanup(struct super_block *sb, struct ptlrpc_request *open_req)
op_data->op_mod_time = get_seconds();
md_close(exp, op_data, NULL, &close_req);
ptlrpc_req_finished(close_req);
- ll_finish_md_op_data(op_data);
+ kfree(op_data);
}

int ll_prep_inode(struct inode **inode, struct ptlrpc_request *req,
@@ -2170,11 +2170,6 @@ struct md_op_data *ll_prep_md_op_data(struct md_op_data *op_data,
return op_data;
}

-void ll_finish_md_op_data(struct md_op_data *op_data)
-{
- kfree(op_data);
-}
-
int ll_show_options(struct seq_file *seq, struct dentry *dentry)
{
struct ll_sb_info *sbi;
--
2.1.0

2015-11-07 07:47:57

by Shivani Bhardwaj

[permalink] [raw]
Subject: [PATCH 9/9] Staging: lustre: llite_internal: Remove function prototype

Remove the prototype of the function ll_finish_md_op_data() as it is no
longer needed.

Signed-off-by: Shivani Bhardwaj <[email protected]>
---
drivers/staging/lustre/lustre/llite/llite_internal.h | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_internal.h b/drivers/staging/lustre/lustre/llite/llite_internal.h
index 157c3284..d12d483 100644
--- a/drivers/staging/lustre/lustre/llite/llite_internal.h
+++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
@@ -792,7 +792,6 @@ struct md_op_data *ll_prep_md_op_data(struct md_op_data *op_data,
struct inode *i1, struct inode *i2,
const char *name, int namelen,
int mode, __u32 opc, void *data);
-void ll_finish_md_op_data(struct md_op_data *op_data);
int ll_get_obd_name(struct inode *inode, unsigned int cmd, unsigned long arg);
char *ll_get_fsname(struct super_block *sb, char *buf, int buflen);
void ll_open_cleanup(struct super_block *sb, struct ptlrpc_request *open_req);
--
2.1.0

2015-11-07 10:45:31

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH 9/9] Staging: lustre: llite_internal: Remove function prototype

This one should have been folded in with the last one. Really the whole
series could have been sent as one patch, but especially the last two
should have been folded together.

regards,
dan carpenter

2015-11-07 15:29:07

by Shivani Bhardwaj

[permalink] [raw]
Subject: Re: [PATCH 9/9] Staging: lustre: llite_internal: Remove function prototype

On Sat, Nov 7, 2015 at 4:15 PM, Dan Carpenter <[email protected]> wrote:
> This one should have been folded in with the last one. Really the whole
> series could have been sent as one patch, but especially the last two
> should have been folded together.
>
> regards,
> dan carpenter
>

Thanks Dan! Could you please suggest whether I should be resending the
complete series?
I'll take care for future patches.

Thank you
Shivani

2015-11-07 20:48:22

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH 9/9] Staging: lustre: llite_internal: Remove function prototype

Don't resend, it's fine. I don't like re-reviewing patches and you
don't like sending them. It's not like it introduces a bug or generate
a warning with our current tools.

But it's still important to understand how the one thing per patch rule
works (don't do half a thing per patch) so I wanted you to be aware of
it for future patches as you said.

regards,
dan carpenter

2015-11-08 08:29:42

by Shivani Bhardwaj

[permalink] [raw]
Subject: Re: [PATCH 9/9] Staging: lustre: llite_internal: Remove function prototype

On Sun, Nov 8, 2015 at 2:17 AM, Dan Carpenter <[email protected]> wrote:
> Don't resend, it's fine. I don't like re-reviewing patches and you
> don't like sending them. It's not like it introduces a bug or generate
> a warning with our current tools.
>
> But it's still important to understand how the one thing per patch rule
> works (don't do half a thing per patch) so I wanted you to be aware of
> it for future patches as you said.
>

I'll take care next time.

Thank you
Shivani

> regards,
> dan carpenter
>

2015-11-09 11:07:34

by Oleg Drokin

[permalink] [raw]
Subject: Re: [PATCH 1/9] Staging: lustre: dir: Replace function calls


On Nov 7, 2015, at 2:41 AM, Shivani Bhardwaj wrote:

> Replace the calls of the function ll_finish_md_op_data() with the
> standard function kfree().

For functions like this that have meaningflul name and also
might include additional logic (even though they don't now),
does it make sense to do this?

I don't feel this is a case of "lustre_specific_kfree_wrapper()"
elimination.
Same for some other wrappers being eliminated, those looked
at times as a way to improve code readability to me.

>
> Signed-off-by: Shivani Bhardwaj <[email protected]>
> ---
> drivers/staging/lustre/lustre/llite/dir.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
> index 5c2ef92..ba3f469 100644
> --- a/drivers/staging/lustre/lustre/llite/dir.c
> +++ b/drivers/staging/lustre/lustre/llite/dir.c
> @@ -182,7 +182,7 @@ static int ll_dir_filler(void *_hash, struct page *page0)
> op_data->op_npages = npages;
> op_data->op_offset = hash;
> rc = md_readpage(exp, op_data, page_pool, &request);
> - ll_finish_md_op_data(op_data);
> + kfree(op_data);
> if (rc < 0) {
> /* page0 is special, which was added into page cache early */
> delete_from_page_cache(page0);
> @@ -363,7 +363,7 @@ struct page *ll_get_dir_page(struct inode *dir, __u64 hash,
> rc = md_enqueue(ll_i2sbi(dir)->ll_md_exp, &einfo, &it,
> op_data, &lockh, NULL, 0, NULL, 0);
>
> - ll_finish_md_op_data(op_data);
> + kfree(op_data);
>
> request = (struct ptlrpc_request *)it.d.lustre.it_data;
> if (request)
> @@ -669,7 +669,7 @@ static int ll_dir_setdirstripe(struct inode *dir, struct lmv_user_md *lump,
> from_kuid(&init_user_ns, current_fsuid()),
> from_kgid(&init_user_ns, current_fsgid()),
> cfs_curproc_cap_pack(), 0, &request);
> - ll_finish_md_op_data(op_data);
> + kfree(op_data);
> if (err)
> goto err_exit;
> err_exit:
> @@ -730,7 +730,7 @@ int ll_dir_setstripe(struct inode *inode, struct lov_user_md *lump,
> /* swabbing is done in lov_setstripe() on server side */
> rc = md_setattr(sbi->ll_md_exp, op_data, lump, lum_size,
> NULL, 0, &req, NULL);
> - ll_finish_md_op_data(op_data);
> + kfree(op_data);
> ptlrpc_req_finished(req);
> if (rc) {
> if (rc != -EPERM && rc != -EACCES)
> @@ -802,7 +802,7 @@ int ll_dir_getstripe(struct inode *inode, struct lov_mds_md **lmmp,
>
> op_data->op_valid = OBD_MD_FLEASIZE | OBD_MD_FLDIREA;
> rc = md_getattr(sbi->ll_md_exp, op_data, &req);
> - ll_finish_md_op_data(op_data);
> + kfree(op_data);
> if (rc < 0) {
> CDEBUG(D_INFO, "md_getattr failed on inode %lu/%u: rc %d\n",
> inode->i_ino,
> @@ -868,7 +868,7 @@ int ll_get_mdt_idx(struct inode *inode)
> op_data->op_flags |= MF_GET_MDT_IDX;
> rc = md_getattr(sbi->ll_md_exp, op_data, NULL);
> mdtidx = op_data->op_mds;
> - ll_finish_md_op_data(op_data);
> + kfree(op_data);
> if (rc < 0) {
> CDEBUG(D_INFO, "md_getattr_name: %d\n", rc);
> return rc;
> @@ -1301,7 +1301,7 @@ static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>
> op_data->op_valid = OBD_MD_FLID;
> rc = md_getattr_name(sbi->ll_md_exp, op_data, &request);
> - ll_finish_md_op_data(op_data);
> + kfree(op_data);
> if (rc < 0) {
> CDEBUG(D_INFO, "md_getattr_name: %d\n", rc);
> goto out_free;
> --
> 2.1.0
>

2015-11-09 12:28:44

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH 1/9] Staging: lustre: dir: Replace function calls

If we ever need the wrapper, we can add it again. These wrappers only
hurt readability because they trick you into thinking there is some
reference counting or something but it's just an ordinary kfree().

regards,
dan carpenter

2015-11-09 13:37:36

by Michał Kępień

[permalink] [raw]
Subject: Re: [PATCH 8/9] Staging: lustre: llite_lib: Remove wrapper function

> Remove the function ll_finish_md_op_data() and replace all its calls
> with the standrd function ll_finish_md_op_data().

I believe you meant to write "standard function kfree()".

--
Best regards,
Michał Kępień

2015-11-09 16:35:48

by Shivani Bhardwaj

[permalink] [raw]
Subject: Re: [PATCH 8/9] Staging: lustre: llite_lib: Remove wrapper function

On Mon, Nov 9, 2015 at 7:07 PM, Michał Kępień <[email protected]> wrote:
>> Remove the function ll_finish_md_op_data() and replace all its calls
>> with the standrd function ll_finish_md_op_data().
>
> I believe you meant to write "standard function kfree()".
>

Yes. I am so sorry. Should I be sending the whole series again?
Thank you
Shivani

> --
> Best regards,
> Michał Kępień

2015-11-09 19:18:21

by Simmons, James A.

[permalink] [raw]
Subject: RE: [PATCH 8/9] Staging: lustre: llite_lib: Remove wrapper function

>On Mon, Nov 9, 2015 at 7:07 PM, Michał Kępień <[email protected]> wrote:
>>> Remove the function ll_finish_md_op_data() and replace all its calls
>>> with the standrd function ll_finish_md_op_data().
>>
>> I believe you meant to write "standard function kfree()".
>>
>
>Yes. I am so sorry. Should I be sending the whole series again?
>Thank you
>Shivani

Yes please redo the series. I saw Oleg's concern and I would recommend that
besides the conversion to kfree that you add comments about what is being deleted.
I.E

/* Free struct md_op_data data*/
kfree(...)
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-11-10 03:09:00

by Shivani Bhardwaj

[permalink] [raw]
Subject: Re: [PATCH 8/9] Staging: lustre: llite_lib: Remove wrapper function

On Tue, Nov 10, 2015 at 12:48 AM, Simmons, James A. <[email protected]> wrote:
>>On Mon, Nov 9, 2015 at 7:07 PM, Michał Kępień <[email protected]> wrote:
>>>> Remove the function ll_finish_md_op_data() and replace all its calls
>>>> with the standrd function ll_finish_md_op_data().
>>>
>>> I believe you meant to write "standard function kfree()".
>>>
>>
>>Yes. I am so sorry. Should I be sending the whole series again?
>>Thank you
>>Shivani
>
> Yes please redo the series. I saw Oleg's concern and I would recommend that
> besides the conversion to kfree that you add comments about what is being deleted.
> I.E
>
> /* Free struct md_op_data data*/
> kfree(...)

I'll do that.
Thank you