Details
-
Bug
-
Resolution: Fixed
-
Major
-
Lustre 2.1.3, Lustre 2.1.4
-
3
-
6116
Description
We have a lot of nodes with a large amount of unreclaimable memory (over 4GB). Whatever we try to do (manually shrinking the cache, clearing lru locks, ...) the memory can't be recovered. The only way to get the memory back is to umount the lustre filesystem.
After some troubleshooting, I was able to wrote a small reproducer where I just open(2) then close(2) files in O_RDWR (my reproducer use to open thousand of files to emphasize the issue).
Attached 2 programs :
- gentree.c (cc -o gentree gentree.c -lpthread) to generate a tree of known files (no need to use readdir in reproducer.c)
- reproducer.c (cc -o reproducer reproduver.c -lpthread) to reproduce the issue.
The macro BASE_DIR has to be adjust according the local cluster configuration (you should provide the name of a directory located on a lustre filesystem).
There is no link between the 2 phases as rebooting the client between gentree & reproducer does't avoid the problem. Running gentree (which open as much files as reproducer) doesn't show the issue.
Attachments
Issue Links
Activity
Andreas, given that we're going to add MSG flag, I'm thinking that if it would be better to pack another transno in reply. Looks pb_last_seen in ptlrpc_body is not used, I think we can use it to carry the last committed on-disk transo (pb_last_committed stores the last committed on-disk/fake transno), so that we can resolve the issue without introducing duplicate transo. I'll update the patch in this way if it's fine with you. Thanks.
Does this affect the case if the duplicate transnos are < last_committed (i.e. the open/close RPCs)? I thought those ones are just replayed on the server? Ah, you are referencing the client code that replays the RPCs...
If it would make the implementation easier, it would be possible to negotiate between the client and server with MSG_* flags in the ptlrpc_body whether they can handle duplicate transno or not. If not, then the server would have to do occasional commits to bump the transno, otherwise this could be avoided for newer clients & servers.
Per previous comments, the client exp_last_committed should be min(last_committed, max(inode_version of all inodes accessed). That ensures that I'd the client is accessing committed inodes that they can be discarded on close (assuming it isn't the very last file created).
Andreas, my point is that last_committed returned to client can't be larger than the exp_last_committed on disk slot, otherwise, client would think server data lost on next recovery (see recovery-small.sh test_54). So, if a client doesn't update disk, it's exp_last_committed on disk should always be zero, and we can't return last_committed larger than 0 in such case. But I think we may force a disk update in this case.
Another problem is that client code assumes transno unique in some places, such as ptlrpc_replay_next():
cfs_list_for_each_safe(tmp, pos, &imp->imp_replay_list) {
req = cfs_list_entry(tmp, struct ptlrpc_request,
rq_replay_list);
/* If need to resend the last sent transno (because a
reconnect has occurred), then stop on the matching
req and send it again. If, however, the last sent
transno has been committed then we continue replay
from the next request. */
if (req->rq_transno > last_transno) {
if (imp->imp_resend_replay)
lustre_msg_add_flags(req->rq_reqmsg,
MSG_RESENT);
break;
}
req = NULL;
}
If there are duplicate transno requests, some requests will be skipped during replay, so the old client will have trouble with this manner.
looks i found one more issue in same area.
mdt save a locks in difficult replies via mdt_object_unlock function and ost via oti_to_request function, but ptlrpc will freed a difficult replies only if last committed larger then export last_commited.
void ptlrpc_commit_replies(struct obd_export *exp)
{
struct ptlrpc_reply_state *rs, *nxt;
DECLARE_RS_BATCH(batch);
ENTRY;
rs_batch_init(&batch);
/* Find any replies that have been committed and get their service
* to attend to complete them. */
/* CAVEAT EMPTOR: spinlock ordering!!! */
spin_lock(&exp->exp_uncommitted_replies_lock);
cfs_list_for_each_entry_safe(rs, nxt, &exp->exp_uncommitted_replies,
rs_obd_list) {
LASSERT (rs->rs_difficult);
/* VBR: per-export last_committed */
LASSERT(rs->rs_export);
if (rs->rs_transno <= exp->exp_last_committed) {
cfs_list_del_init(&rs->rs_obd_list);
rs_batch_add(&batch, rs);
}
}
spin_unlock(&exp->exp_uncommitted_replies_lock);
rs_batch_fini(&batch);
EXIT;
}
but exp_last_commited is zero until modification request is hit but we have send a locks via getattr request without any transaction opened.
Per previous comments, the client exp_last_committed should be min(last_committed, max(inode_version of all inodes accessed). That ensures that I'd the client is accessing committed inodes that they can be discarded on close (assuming it isn't the very last file created).
Niu, for second option I thought the client would not even try to put the RPC into the replay list if transno <= last_committed?
That's ture for close request but not ture for open request, open request has to be retained regardless of transno.
Is it not checked in the close RPC callback to drop both requests from this list if both transnos are <= last_committed?
No, we didn't do that, but it's easy to fix if we change the client code.
Iff a fake transno is given to the client to avoid a duplicate open+create transno, it should be inode_version+1, and upon close if the real last_committed is >= inode_version+1 then it should send inode_version+1 back to the client for last_committed.
One thing can't be handled well by this manner is: If a client never do update operation, it'll always have zero exp_last_committed, which means any replied trasno can't be larger than 0... We may fix it by generating an update operation upon each client connection (connect becomes an update operation? that looks quite dirty to me), or we should just ignore this rare case (no any update from a client) for this moment? Any suggestion? Thanks.
Niu, for second option I thought the client would not even try to put the RPC into the replay list if transno <= last_committed? Is it not checked in the close RPC callback to drop both requests from this list if both transnos are <= last_committed? Iff a fake transno is given to the client to avoid a duplicate open+create transno, it should be inode_version+1, and upon close if the real last_committed is >= inode_version+1 then it should send inode_version+1 back to the client for last_committed.
In any case, I don't mind also fixing this on the client as long as it doesn't break the interoperability. The client should be smart enough to cancel matching pairs of open+close if they are <= last_committed. That won't solve the problem by itself, but in conjunction with the server changes not to invent fake transnos > last_committed it should work.
I think this is a combination of two bugs - server giving out transnos that are > last_committed and never committing them, and also that the client does not drop RPCs if the last_committed doesn't change. This should be possible to handle without scanning the list each time I think.
Thank you, Andreas.
Looks the first option needs to change client (or even protocol), I think if we have to change client & protocol, we'd choose some cleaner & easier way: reply back two last_committed transnos (last_committed & on-disk last_committed, similar to my patchset 1) or totally change the open replay mechanism as you mentioned.
I tried the second way today and discovered another problem: for performance reason, client only tries to scan the replay list to free the committed/closed requests whenever the last_committed is bumped, which means without client code changes, the open requests won't be freed promptly, even they have transno smaller than last_committed. Even if we change the client code, we'd think out some better way rather than scanning the replay list on every reply.
So, as a short term solution, I think we'd go back to keep using my patchset 2 (update disk on server periodically), or ask user to do this in userspace by themselves? What do you think about?
Three options would be:
- client B gets back same data to do replay as client A, so it doesn't matter which one is replayed. However, this might get more complex to handle, but has the benefit of increasing robustness of the replay.
- client B gets back a fake transno if openA is uncommitted, like it does today. This means there is still a commit to be done, do last_committed will be increased to either cover this fake transno, or at worst the number of fake transno is limited to the few seconds until commit.
- commit on share of the open file. I don't like this much, because it slows down the common code path.
Niu, Andreas, this become more and more complex as I can see. The problem we are trying to solve is that client keeps closed open requests in replay queue, right? Meanwhile the client itself wants them to be dropped from that queue, see mdc_close():
So the problem is that request is not dropped when client ask to do that. That is because of last_committed check which is the only mechanism to drop request and that means for me we just need to add another one to drop request from replay queue regardless its transno. E.g. rq_closed_open flag can be added and checked to drop request. That could be much simpler. Did I miss something and there are other cases we are trying to solve?