gmane.comp.file-systems.ocfs2.devel

[PATCH] ocfs2: Free sc->sc_page in sc_kref_release()v2

Younger Liu — 2013-06-18T06:02:59

There is a memory leak in sc_kref_release().
When free struct o2net_sock_container (sc), 
we should release sc->sc_page.

Signed-off-by: Younger Liu <younger.liu< at >huawei.com>
Reviewed-by: Jie Liu <jeff.liu< at >oracle.com>
---
 fs/ocfs2/cluster/tcp.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
index aa88bd8..bb9a48b 100644
--- a/fs/ocfs2/cluster/tcp.c
+++ b/fs/ocfs2/cluster/tcp.c
< at >< at > -406,6 +406,9 < at >< at > static void sc_kref_release(struct kref *kref)
 sc->sc_node = NULL;
 
 o2net_debug_del_sc(sc);
+
+if (sc->sc_page)
+__free_page(sc->sc_page);
 kfree(sc);
 }

Re: [PATCH] ocfs2: Free sc->sc_page insc_kref_release()

Younger Liu — 2013-06-18T05:57:31

Thanks for your review comments.
I will resend this patch.

On 2013/6/18 12:35, Jeff Liu wrote:

Re: [PATCH] ocfs2: Free sc->sc_page insc_kref_release()

Jeff Liu — 2013-06-18T04:35:14


Nice catch. I have not reacted from another thing in process at that time.

Thanks,
-Jeff

Re: [PATCH] ocfs2: Free sc->sc_page insc_kref_release()

Li Zefan — 2013-06-18T04:29:07

But why set sc->sc_page to NULL, given sc is to be kfreed.

Re: [PATCH] ocfs2: Free sc->sc_page insc_kref_release()

Jeff Liu — 2013-06-18T04:20:02


Looks fine to me, thanks!

Reviewed-by: Jie Liu <jeff.liu< at >oracle.com>

[PATCH] ocfs2: Free sc->sc_page in sc_kref_release()

Younger Liu — 2013-06-18T03:40:32

There is a memory leak in sc_kref_release().
When free struct o2net_sock_container (sc), 
we should release sc->sc_page.

Signed-off-by: Younger Liu <younger.liu< at >huawei.com>
---
 fs/ocfs2/cluster/tcp.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
index aa88bd8..f0272b9 100644
--- a/fs/ocfs2/cluster/tcp.c
+++ b/fs/ocfs2/cluster/tcp.c
< at >< at > -406,6 +406,11 < at >< at > static void sc_kref_release(struct kref *kref)
 sc->sc_node = NULL;
 
 o2net_debug_del_sc(sc);
+
+if (sc->sc_page) {
+__free_page(sc->sc_page);
+sc->sc_page = NULL;
+}
 kfree(sc);
 }

Re: [PATCH] Fix waiting status race condition in dlm recovery V2

Xue jiufei — 2013-06-18T03:13:14

Hi, xiaowei,
We have reviewed this patch and have some questions:
1) in dlm_is_node_in_livemap(), I think it should use 
!!(test_bit(node, dlm->live_nodes_map)) to determine whether a node is
live;
2) why not use dlm_is_node_dead() instead of dlm_is_node_in_livemap()
in dlm_remaster_locks?
I think dlm_is_node_dead() is better because dlm->live_nodes_map
may be remain when another node umount.

Thanks.





.

Re: [PATCH] unlink performance: wait for open lock in case of dirs

Goldwyn Rodrigues — 2013-06-18T02:17:00

This is best answered by the DLM. In short, a direct EX grant goes to
the lock master and if no previous lock found, is granted. In the case
of PR->EX, it first goes to the lock master, the lock master checks
the holders, request each holder to unlock. Once it gets a response
from all PR holders only then grants the lock.


The locking sequence in inode.c:ocfs2_read_locked_inode is open_lock
and then meta locks (ocfs2_inode_lock).
The ocfs2_try_open_lock was a non-blocking sequence and was always
called with meta locks held. However, if we use a blocking open_lock,
it will deadlock. OTOH, if we already posses the PR lock, it means
that the lock sequence already has happened in open_lock->meta_lock
and hence this is just an upgrade from PR->EX hence we can wait.


Attached. You will have to modify the name of the other nodes in the
script. In my case, it was "sles11a" and "sles11c". Also remember to
setup .ssh/authorized_keys for automatic logins. You may get delays in
later calls because ocfs2cmt thread wak

Re: [PATCH] unlink performance: wait for open lock in case of dirs

shencanquan — 2013-06-18T01:30:06



Why a PR->EX convert takes longer than a simple EX grant.


why it can avoid the ABBA locking.


Please send the testcase . thanks.

Re: [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

shencanquan — 2013-06-18T01:03:46



I think it maybe impact performance. because it use the cluster lock and
it maybe flush the inode cache to disk and read the inode from disk. but
I think the correct function of llseek is more important.

Re: [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

shencanquan — 2013-06-18T00:57:39


 Thanks for your comments.



yes. it need to protect the offset adjustment.


Thanks you. I will see the document .

Re: [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

Jeff Liu — 2013-06-17T15:17:49


Can you please wrap these around 72 columns or so in the future?
Also, that would be appreciated if you can supply more detailed
info to reflect the results before/after applying this patch.


Please add spaces between comments and annotation, i.e,
   /*  Need to ...... */


Why not protect the offset adjustment insides ocfs2 inode locks?


Looks your email client has not configured properly for posting patches
because all those changes are not using tab for code indentation, please
refer to Documentation/email-clients.txt for detail if so.

-Jeff

Re: [PATCH] Add bits_wanted while calculating credits in ocfs2_calc_extend_credits

Jeff Liu — 2013-06-17T14:40:32


Looks good to me, thank you!
Reviewed-by: Jie Liu <jeff.liu< at >oracle.com>

[PATCH] Add bits_wanted while calculating credits in ocfs2_calc_extend_credits

Goldwyn Rodrigues — 2013-06-17T14:08:24

While adding extends to a file, the credits are calculated incorrectly
and if the requested clusters is more than one (or more because we used 
a conservative limit) then we run out of journal credits and we hit
an assert in journalling code.

The function parameter bits_wanted variable was not used at all.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn< at >suse.com>

--- 
diff --git a/fs/ocfs2/journal.h b/fs/ocfs2/journal.h
index a3385b6..7d927cd 100644
--- a/fs/ocfs2/journal.h
+++ b/fs/ocfs2/journal.h
< at >< at > -538,7 +538,7 < at >< at > static inline int ocfs2_calc_extend_credits(struct super_block *sb,
 extent_blocks = 1 + 1 + le16_to_cpu(root_el->l_tree_depth);
 
 return bitmap_blocks + sysfile_bitmap_blocks + extent_blocks +
-       ocfs2_quota_trans_credits(sb);
+       ocfs2_quota_trans_credits(sb) + bits_wanted;
 }
 
 static inline int ocfs2_calc_symlink_credits(struct super_block *sb)

Re: [PATCH 1/2] ocfs2: Fix llseek() semantics and dosome cleanup

Jeff Liu — 2013-06-16T07:00:24


Hmm??  Did you mean to say that an invalid whence(i.e, whence > SEEK_MAX)
can be passed into generic_file_llseek()?
If so, I don't think that is a problem because any invalid whence should
be rejected at the entrance of VFS lseek(2) with EINVAL.

Thanks,
-Jeff

Re: [PATCH 1/2] ocfs2: Fix llseek() semantics and dosome cleanup

Jeff Liu — 2013-06-15T05:09:18

[Add ocfs2-devel to CC-list]

Hello Richard,

Thanks for your patch.

On 06/15/2013 03:23 AM, Richard Yao wrote:


Agree, but please see my comments below.


I have another patch set for refactoring ocfs2_file_llseek() but not yet found time
to run a comprehensive tests.  It can solve the existing issues but also improved the
SEEK_DATA/SEEK_HOLE for unwritten extents, i.e. OCFS2_EXT_UNWRITTEN.

With this change, SEEK_DATA/SEEK_HOLE will go into separate function with a little code
duplication instead of the current mix-ups in ocfs2_seek_data_hole_offset(), i.e, 

loff_t ocfs2_file_llseek()
{
switch (origin) {
        case SEEK_END:
        case SEEK_CUR:
        case SEEK_SET:
                return generic_file_llseek(file, offset, origin);
        case SEEK_DATA:
                return ocfs2_seek_data(file, offset);
        case SEEK_HOLE:
                return ocfs2_seek_hole(file, offset);
        default:
                return -EINVAL;
        }
}

I personally like keeping SEEK_DATA/SEEK_HOLE in swi

[PATCH] unlink performance: wait for open lock incase of dirs

Goldwyn Rodrigues — 2013-06-15T02:05:10

This patch is to improve the unlink performance. Here is the scenario:

On node A, create multiple directories say d1-d8, and each have 3
files under it f1, f2 and f3.
On node B, delete all directories using rm -Rf d*

The FS first unlinks f1, f2 and f3. However, when it performs
ocfs2_evict_inode() -> ocfs2_delete_inode() ->
ocfs2_query_inode_wipe() -> ocfs2_try_open_lock() on d1, it fails
with -EAGAIN. The open lock fails because on the remote node
a PR->EX convert takes longer than a simple EX grant.

This starts a checkpoint because OCFS2_INODE_DELETED flag is not set
on the directory inode. Now, a checkpoint interferes with the journaling
of the inodes deleted in the following unlinks, in our case,
directories d2-d8 and the files contained in it.

With this patch, We wait on a directory EX lock only if we already
have an open_lock in PR mode. This way we will avoid the ABBA locking.

By waiting for the open_lock on the directory, I am getting a unlink
performance improvement of a rm -Rf of 50-60% in the

Re: unlink: why does try_open_lock fail on directory but works for a file?

shencanquan — 2013-06-13T02:03:10

     No , I think the directories use the open_lock. because the 
open_lock use to check the other node  had opened the file or directory 
in the ocfs2 cluster. if yes , it can't delete the file or directory on 
the disk.

Re: unlink: why does try_open_lock fail on directory but works for a file?

Goldwyn Rodrigues — 2013-06-10T12:11:43

This is a self-fabricated test and I just created the directory using
mkdir and files using touch Yes, it is possible that the PR open lock
is still on the directory.


Do you mean to say directories don't use open_lock. Could you provide
the code/design reference?

Re: unlink: why does try_open_lock fail on directory but works for a file?

shencanquan — 2013-06-10T10:09:38

  on directory d1 it fail to call ocfs2_try_open_lock, it mean other 
node has lock the open lock. it maybe node A has open the directory d1?
   I don't think that the directory don't use the open_lock, otherwise 
it will be success to call the ocfs2_try_open_lock.

Re: [PATCH v2] ocfs2: fix mutex_unlock and possible memory leak in ocfs2_remove_btree_range

Jeff Liu — 2013-06-10T08:19:01


Reviewed-by: Jie Liu <jeff.liu< at >oracle.com>