Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-6529

Server side lock limits to avoid unnecessary memory exhaustion

Details

    • Bug
    • Resolution: Fixed
    • Critical
    • Lustre 2.8.0
    • None
    • 3
    • 9223372036854775807

    Description

      As seen in tickets like LU-5727, we currently rely almost entirely on the good aggregate behavior of the lustre clients to avoid memory exhaustion on the MDS (and other servers, no doubt).

      We require the servers, the MDS in particular, to instead limit ldlm lock usage to something reasonable to avoid OOM conditions on their own. It is not good design to leave the MDS's memory usage entirely up to the very careful administrative limiting of ldlm lock usage limits across all of the client nodes.

      Consider that some sites have many thousands of clients across many clusters where such careful balancing and coordinated client limits may be difficult to achieve. Consider also WAN usages, where some clients might not ever reside at the same organization as the servers. Consider also bugs in the client, again like LU-5727.

      See also the attached graph showing MDS memory usage. Clearly the ldlm lock usage grows without bound, and other parts of the kernel memory usage are put under undue pressure. 70+ GiB of ldlm lock usage is not terribly reasonable for our setup.

      Some might argue that the SLV code needs to be fixed, and I have no argument against pursuing that work. That could certainly be worked in some other ticket.

      But even if SLV is fixed, we still require enforcement of good memory usage on the server side. There will always be client bugs or misconfiguration on clients and the server OOMing is not a reasonable response to those issues.

      I would propose a configurable hard limit on the number of locks (or space used by locks) on the server side.

      I am open to other solutions, of course.

      Attachments

        Issue Links

          Activity

            [LU-6529] Server side lock limits to avoid unnecessary memory exhaustion

            Patches. Yes.

            morrone Christopher Morrone (Inactive) added a comment - Patches. Yes.

            Chris, do we have this patch in our local release?

            marc@llnl.gov D. Marc Stearman (Inactive) added a comment - Chris, do we have this patch in our local release?

            All patches have landed for 2.8.

            jgmitter Joseph Gmitter (Inactive) added a comment - All patches have landed for 2.8.

            Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/16123/
            Subject: LU-6529 ldlm: improve proc interface of lock reclaim
            Project: fs/lustre-release
            Branch: master
            Current Patch Set:
            Commit: 33b55f223a42f20916bc417f7e5a21f68b59cd02

            gerrit Gerrit Updater added a comment - Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/16123/ Subject: LU-6529 ldlm: improve proc interface of lock reclaim Project: fs/lustre-release Branch: master Current Patch Set: Commit: 33b55f223a42f20916bc417f7e5a21f68b59cd02
            niu Niu Yawei (Inactive) added a comment - - edited

            Another bug: The proc files accept negative values. Negative values should be rejected.

            Well, it's same as other Lustre proc files which rely on some basic helper functions. I think it's worth a new ticket to fix this.

            I am also disappointed that the patch passed review with so little in the way of function comments. Aren't function comments are landing requirement?

            I'll try to add more comments in the next patch.

            Under Lustre 2.5.4 + local patches, we seem to be hitting the high lock limit prematurely, at least as far as we can tell from the number of ldlm_locks active on the slab. Is there some other way to get an idea of what the lustre server thinks is the current lock count?

            There is a counter for the ldlm lock, but it's not exported, maybe I'd export it via proc for debug purpose (together with the proc interface changes).

            In current Lustre, you can roughly get the number by adding up the /proc/fs/lustre/ldlm/namespaces/$target/pool/granted for all the MDT/OST targets on server.

            niu Niu Yawei (Inactive) added a comment - - edited Another bug: The proc files accept negative values. Negative values should be rejected. Well, it's same as other Lustre proc files which rely on some basic helper functions. I think it's worth a new ticket to fix this. I am also disappointed that the patch passed review with so little in the way of function comments. Aren't function comments are landing requirement? I'll try to add more comments in the next patch. Under Lustre 2.5.4 + local patches, we seem to be hitting the high lock limit prematurely, at least as far as we can tell from the number of ldlm_locks active on the slab. Is there some other way to get an idea of what the lustre server thinks is the current lock count? There is a counter for the ldlm lock, but it's not exported, maybe I'd export it via proc for debug purpose (together with the proc interface changes). In current Lustre, you can roughly get the number by adding up the /proc/fs/lustre/ldlm/namespaces/$target/pool/granted for all the MDT/OST targets on server.

            Under Lustre 2.5.4 + local patches, we seem to be hitting the high lock limit prematurely, at least as far as we can tell from the number of ldlm_locks active on the slab. Is there some other way to get an idea of what the lustre server thinks is the current lock count?

            morrone Christopher Morrone (Inactive) added a comment - Under Lustre 2.5.4 + local patches, we seem to be hitting the high lock limit prematurely, at least as far as we can tell from the number of ldlm_locks active on the slab. Is there some other way to get an idea of what the lustre server thinks is the current lock count?

            I am also disappointed that the patch passed review with so little in the way of function comments. Aren't function comments are landing requirement?

            morrone Christopher Morrone (Inactive) added a comment - I am also disappointed that the patch passed review with so little in the way of function comments. Aren't function comments are landing requirement?

            Another bug: The proc files accept negative values. Negative values should be rejected.

            morrone Christopher Morrone (Inactive) added a comment - Another bug: The proc files accept negative values. Negative values should be rejected.

            Niu Yawei (yawei.niu@intel.com) uploaded a new patch: http://review.whamcloud.com/16123
            Subject: LU-6529 ldlm: improve proc interface of lock reclaim
            Project: fs/lustre-release
            Branch: master
            Current Patch Set: 1
            Commit: cd15ce613f958e14fd8c8a01a97cdd67cb17e249

            gerrit Gerrit Updater added a comment - Niu Yawei (yawei.niu@intel.com) uploaded a new patch: http://review.whamcloud.com/16123 Subject: LU-6529 ldlm: improve proc interface of lock reclaim Project: fs/lustre-release Branch: master Current Patch Set: 1 Commit: cd15ce613f958e14fd8c8a01a97cdd67cb17e249

            I agree with that watermark numbers are confusion (numbers written are not equal to the numbers reading back), I'll change it, and I'm fine to change the name as you suggested.

            For the case that we are about talking about here in this ticket, I disagree rather strongly with the claim that letting the clients decide on the lock reclaim is "ideal". It would only be reasonable to let the clients decide when the server is not under memory pressure. This entire ticket is explicitly about dealing with the situation where the server is under memory pressure.

            Maybe the wording of the comment isn't accurate, what I want to expressed is that choosing locks to be reclaimed should be done by client (because client knows better which locks are not used), server of course should decide when to start lock reclaim (because server knows memory pressure). It's not ideal, but I think it's the best we can do so far. Anyway, given it's misleading, I'm going to remove it.

            Ideal would be to integrate with the kernel shrinker instead of having these thresholds. When kernel pressure is high enough, using additional memory to send messages to the clients and then waiting a potentially very long time for a response is simply not reasonable. The server will always need to be able to summarily discard locks.

            I think that's the point why we should always have an upper threshold, it's the safe guarding for server to not enter the situation you mentioned (under severe memory pressure, don't have enough memory to revoke locks), it may not enough, and we can add other restrictions later if necessary. The lower threshold may be removed if we integrate with the kernel shrinker in the future (Let shrinker to take over in non-urgent situation).

            Discarding locks means some clients have to be evicted, I think that's the case we must try to avoid. (by some other manners, set the upper limit for instance)

            niu Niu Yawei (Inactive) added a comment - I agree with that watermark numbers are confusion (numbers written are not equal to the numbers reading back), I'll change it, and I'm fine to change the name as you suggested. For the case that we are about talking about here in this ticket, I disagree rather strongly with the claim that letting the clients decide on the lock reclaim is "ideal". It would only be reasonable to let the clients decide when the server is not under memory pressure. This entire ticket is explicitly about dealing with the situation where the server is under memory pressure. Maybe the wording of the comment isn't accurate, what I want to expressed is that choosing locks to be reclaimed should be done by client (because client knows better which locks are not used), server of course should decide when to start lock reclaim (because server knows memory pressure). It's not ideal, but I think it's the best we can do so far. Anyway, given it's misleading, I'm going to remove it. Ideal would be to integrate with the kernel shrinker instead of having these thresholds. When kernel pressure is high enough, using additional memory to send messages to the clients and then waiting a potentially very long time for a response is simply not reasonable. The server will always need to be able to summarily discard locks. I think that's the point why we should always have an upper threshold, it's the safe guarding for server to not enter the situation you mentioned (under severe memory pressure, don't have enough memory to revoke locks), it may not enough, and we can add other restrictions later if necessary. The lower threshold may be removed if we integrate with the kernel shrinker in the future (Let shrinker to take over in non-urgent situation). Discarding locks means some clients have to be evicted, I think that's the case we must try to avoid. (by some other manners, set the upper limit for instance)

            I am reopening this ticket, because I think there are a few things about the patch that really should be addressed before Lustre 2.8 is released.

            First of all, the number that one writes to the waterfall files is not necessarily the number that one reads back. This is going to be very confusing for users. The problem seems to be some kind of rounding error, where the code winds up rounding down to the closes multiple of 1 MiB. Especially confusing is to write "1M" to the file and then read back "0". "0" means that the threshold is disabled, so that is really not acceptable.

            Instead of doing multiple steps of math (including do_div) on the number supplied by the user, the number supplied by the user needs to be recorded unmolested and supplied back to the user when they read from the file.

            Next, it was pointed out to me that "watermark_mb_low" is not a terribly good name. That implies that the number can't go any lower. Perhaps better names for these files would be something like:

            • lock_limit_mb
            • reclaim_threshold_mb

            I think that might be clearer to users that "reclaim_threshold_mb" is the point where reclaim kicks in, and "lock_limit_mb" is the limit beyond which no further locks will be permitted.

            Next there is this comment, which I think is misleading:

            /*

            • FIXME:
              *
            • In current implementation, server identifies which locks should be
            • revoked by choosing locks from namespace/resource in a roundrobin
            • manner, which isn't optimal. The ideal way should be server notifies
            • clients to cancel locks voluntarily, because only client knows exactly
            • when the lock is last used.

            For the case that we are about talking about here in this ticket, I disagree rather strongly with the claim that letting the clients decide on the lock reclaim is "ideal". It would only be reasonable to let the clients decide when the server is not under memory pressure. This entire ticket is explicitly about dealing with the situation where the server is under memory pressure.

            Ideal would be to integrate with the kernel shrinker instead of having these thresholds. When kernel pressure is high enough, using additional memory to send messages to the clients and then waiting a potentially very long time for a response is simply not reasonable. The server will always need to be able to summarily discard locks.

            morrone Christopher Morrone (Inactive) added a comment - I am reopening this ticket, because I think there are a few things about the patch that really should be addressed before Lustre 2.8 is released. First of all, the number that one writes to the waterfall files is not necessarily the number that one reads back. This is going to be very confusing for users. The problem seems to be some kind of rounding error, where the code winds up rounding down to the closes multiple of 1 MiB. Especially confusing is to write "1M" to the file and then read back "0". "0" means that the threshold is disabled, so that is really not acceptable. Instead of doing multiple steps of math (including do_div) on the number supplied by the user, the number supplied by the user needs to be recorded unmolested and supplied back to the user when they read from the file. Next, it was pointed out to me that "watermark_mb_low" is not a terribly good name. That implies that the number can't go any lower. Perhaps better names for these files would be something like: lock_limit_mb reclaim_threshold_mb I think that might be clearer to users that "reclaim_threshold_mb" is the point where reclaim kicks in, and "lock_limit_mb" is the limit beyond which no further locks will be permitted. Next there is this comment, which I think is misleading: /* FIXME: * In current implementation, server identifies which locks should be revoked by choosing locks from namespace/resource in a roundrobin manner, which isn't optimal. The ideal way should be server notifies clients to cancel locks voluntarily, because only client knows exactly when the lock is last used. For the case that we are about talking about here in this ticket, I disagree rather strongly with the claim that letting the clients decide on the lock reclaim is "ideal". It would only be reasonable to let the clients decide when the server is not under memory pressure. This entire ticket is explicitly about dealing with the situation where the server is under memory pressure. Ideal would be to integrate with the kernel shrinker instead of having these thresholds. When kernel pressure is high enough, using additional memory to send messages to the clients and then waiting a potentially very long time for a response is simply not reasonable. The server will always need to be able to summarily discard locks.

            People

              niu Niu Yawei (Inactive)
              morrone Christopher Morrone (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: