[Cluster-devel] [GFS2 PATCH 2/3] GFS2: Drop initial reservation if too small

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| On Mon, 2013-09-16 at 15:10 -0500, Bob Peterson wrote:
| > Before this patch, function gfs2_inplace_reserve would grab a blocking
| > lock on the rgrp glock. This is not necessary if we already know that
| > rgrp doesn't have enough blocks to satisfy the request. This patch
| > makes the function drop the inadequate reservation so that a more
| > suitable rgrp may be found (with a try lock). This is not a very
| > important thing to do, and it still requires a blocking lock to remove
| > the reservation. However, it allows us to further simplify the code
| > with a future patch because after this patch, we will know for sure
| > that the reservation will not be in the rgrp's reservations tree
| > unless it has enough blocks.
| > ---
| >  fs/gfs2/rgrp.c | 45 +++++++++++++++++++++++++++++++++++++++++----
| >  1 file changed, 41 insertions(+), 4 deletions(-)
| > 
| This looks like the wrong thing to do. If we have a reservation, then we
| should always use it until it is used up, otherwise we are adding more
| fragmentation when we do not need to. The requested amount is supposed
| to be the number of blocks that we'd like to have in an ideal world, but
| provided we can supply the minimum number of blocks (currently 1) then
| there is no need to drop the reservation.
| 
| Also, I don't want to reintroduce the try locks now that we've
| eliminated them from this part of the code, since that brings back
| another set of problems. Maybe I'm not understanding what the intent is
| here though as despite the comment above, the patches don't appear to
| actually bring the try lock back.
| 
| postmark only tends to write very small files, depending on what
| settings it is being run with, so will likely not gain from the
| reservations work at all. The main reason for using it is to ensure that
| we don't get a regression rather than expecting it to show any
| improvements.
| 
| Steve.

Hi,

Actually, unless I'm reading the code wrong, it already does this.
If this is the wrong thing to do, we need to change the logic.
In today's gfs2_inplace_reserve, there's a check:

		if (rs->rs_rbm.rgd->rd_free_clone >= requested) {
			ip->i_rgd = rs->rs_rbm.rgd;
			return 0;
		}
                (otherwise it gets rejected)

This similarly rejects any pre-existing reservation if it doesn't satisfy
the requested size. What I was trying to do here is preserve the existing
logic. If it's wrong and we need to change it, that's fine, but I have
doubts that it will pass all the testing; when I was working on the
block reservations code, I believe I tried that and got into some nasty
problems. Still, it may be worth trying again.

Regards,

Bob Peterson
Red Hat File Systems