[Cluster-devel] [PATCH] dlm: remove unnecessary error check
We don't need the redundant logic since send_message always returns 0. Signed-off-by: Guoqing Jiang gqji...@suse.com --- fs/dlm/lock.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 35502d4..6fc3de9 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct dlm_lkb *lkb, int mstype) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, msg_reply_type(mstype)); @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct dlm_lkb *lkb) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); -- 1.7.12.4
Re: [Cluster-devel] [GFS2 PATCH] gfs2: Don't support fallocate on jdata files
- Original Message - We cannot provide an efficient implementation due to the headers on the data blocks, so there doesn't seem much point in having it. Resolves: rhbz#1221331 Signed-off-by: Abhi Das a...@redhat.com --- fs/gfs2/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index c706c6d..8252115 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -917,7 +917,7 @@ static long gfs2_fallocate(struct file *file, int mode, loff_t offset, loff_t le struct gfs2_holder gh; int ret; - if (mode ~FALLOC_FL_KEEP_SIZE) + if ((mode ~FALLOC_FL_KEEP_SIZE) || gfs2_is_jdata(ip)) return -EOPNOTSUPP; mutex_lock(inode-i_mutex); -- 1.8.1.4 Hi, Now that all the questions have been answered, I pushed this patch to the for-next branch of the linux-gfs2 tree: https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?h=for-nextid=86066914edff2316cbed63aac8a87d5001441a16 Regards, Bob Peterson Red Hat File Systems
Re: [Cluster-devel] [GFS2 PATCH] GFS2: Don't brelse rgrp buffer_heads every allocation
- Original Message - Hi, On 05/06/15 15:49, Bob Peterson wrote: Hi, This patch allows the block allocation code to retain the buffers for the resource groups so they don't need to be re-read from buffer cache with every request. This is a performance improvement that's especially noticeable when resource groups are very large. For example, with 2GB resource groups and 4K blocks, there can be 33 blocks for every resource group. This patch allows those 33 buffers to be kept around and not read in and thrown away with every operation. The buffers are released when the resource group is either synced or invalidated. The blocks should be cached between operations, so this should only be resulting in a skip of the look up of the cached block, and no changes to the actual I/O. Does that mean that grab_cache_page() is slow I wonder? Or is this an issue of going around the retry loop due to lack of memory at some stage? How does this interact with the rgrplvb support? I'd guess that with that turned on, this is no longer an issue, because we'd only read in the blocks for the rgrps that we are actually going to use? Steve. Hi, If you compare the two vmstat outputs in the bugzilla #1154782, you'll see no significant difference in memory usage nor cpu usage. So I assume the page lookup is the slow part; not because it's such a slow thing but because it's done 33 times per read-reference-invalidate (33 pages to look up per rgrp). Regards, Bob Peterson Red Hat File Systems
Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check
- Original Message - We don't need the redundant logic since send_message always returns 0. Signed-off-by: Guoqing Jiang gqji...@suse.com --- fs/dlm/lock.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 35502d4..6fc3de9 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct dlm_lkb *lkb, int mstype) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, msg_reply_type(mstype)); @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct dlm_lkb *lkb) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); -- 1.7.12.4 Hi, The patch looks okay, but if remove_from_waiters() always returns 0, wouldn't it be better to change the function from int to void and return 0 here? The advantage is that code spelunkers wouldn't need to back-track one more level (not to mention the instruction or two it might save). Regards, Bob Peterson Red Hat File Systems
Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check
Hi Bob, Bob Peterson wrote: - Original Message - We don't need the redundant logic since send_message always returns 0. Signed-off-by: Guoqing Jiang gqji...@suse.com --- fs/dlm/lock.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 35502d4..6fc3de9 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct dlm_lkb *lkb, int mstype) send_args(r, lkb, ms); -error = send_message(mh, ms); -if (error) -goto fail; -return 0; +return send_message(mh, ms); fail: remove_from_waiters(lkb, msg_reply_type(mstype)); @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct dlm_lkb *lkb) send_args(r, lkb, ms); -error = send_message(mh, ms); -if (error) -goto fail; -return 0; +return send_message(mh, ms); fail: remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); -- 1.7.12.4 Hi, The patch looks okay, but if remove_from_waiters() always returns 0, wouldn't it be better to change the function from int to void and return 0 here? The advantage is that code spelunkers wouldn't need to back-track one more level (not to mention the instruction or two it might save). Seems remove_from_waiters is not always returns 0, the return value could be -1 or 0 which depends on _remove_from_waiters. BTW, I found that there are no big difference between send_common and send_lookup, since the send_common can also be use to send lookup message, I guess send_lookup can be removed as well. Thanks, Guoqing
Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check
- Original Message - Hi Bob, Bob Peterson wrote: - Original Message - We don't need the redundant logic since send_message always returns 0. Signed-off-by: Guoqing Jiang gqji...@suse.com --- fs/dlm/lock.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 35502d4..6fc3de9 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct dlm_lkb *lkb, int mstype) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, msg_reply_type(mstype)); @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct dlm_lkb *lkb) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); -- 1.7.12.4 Hi, The patch looks okay, but if remove_from_waiters() always returns 0, wouldn't it be better to change the function from int to void and return 0 here? The advantage is that code spelunkers wouldn't need to back-track one more level (not to mention the instruction or two it might save). Seems remove_from_waiters is not always returns 0, the return value could be -1 or 0 which depends on _remove_from_waiters. BTW, I found that there are no big difference between send_common and send_lookup, since the send_common can also be use to send lookup message, I guess send_lookup can be removed as well. Thanks, Guoqing Hi Guoqing, If remove_from_waiters can return -1, then the patch would prevent the code from calling remove_from_waiters. So the patch still doesn't look right to me. Regards, Bob Peterson Red Hat File Systems
Re: [Cluster-devel] [PATCH] dlm: remove unnecessary error check
Bob Peterson wrote: - Original Message - Hi Bob, Bob Peterson wrote: - Original Message - We don't need the redundant logic since send_message always returns 0. Signed-off-by: Guoqing Jiang gqji...@suse.com --- fs/dlm/lock.c | 10 ++ 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c index 35502d4..6fc3de9 100644 --- a/fs/dlm/lock.c +++ b/fs/dlm/lock.c @@ -3656,10 +3656,7 @@ static int send_common(struct dlm_rsb *r, struct dlm_lkb *lkb, int mstype) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, msg_reply_type(mstype)); @@ -3763,10 +3760,7 @@ static int send_lookup(struct dlm_rsb *r, struct dlm_lkb *lkb) send_args(r, lkb, ms); - error = send_message(mh, ms); - if (error) - goto fail; - return 0; + return send_message(mh, ms); fail: remove_from_waiters(lkb, DLM_MSG_LOOKUP_REPLY); -- 1.7.12.4 Hi, The patch looks okay, but if remove_from_waiters() always returns 0, wouldn't it be better to change the function from int to void and return 0 here? The advantage is that code spelunkers wouldn't need to back-track one more level (not to mention the instruction or two it might save). Seems remove_from_waiters is not always returns 0, the return value could be -1 or 0 which depends on _remove_from_waiters. BTW, I found that there are no big difference between send_common and send_lookup, since the send_common can also be use to send lookup message, I guess send_lookup can be removed as well. Thanks, Guoqing Hi Guoqing, If remove_from_waiters can return -1, then the patch would prevent the code from calling remove_from_waiters. So the patch still doesn't look right to me. Hi Bob, The remove_from_waiters could only be invoked after failed to create_message, right? Since send_message always returns 0, this patch doesn't touch anything about the failure path, and it also doesn't change the original semantic. Thanks, Guoqing
