From mboxrd@z Thu Jan  1 00:00:00 1970
From: Steven Whitehouse <swhiteho@redhat.com>
Date: Mon, 11 Dec 2017 12:12:05 +0000
Subject: [Cluster-devel] [GFS2 PATCH] GFS2: Log the reason for log
 flushes in every log header
In-Reply-To: <5adc2737-1f11-27b1-eb46-733fdd9ab9df@redhat.com>
References: <94557090.39942461.1512747048003.JavaMail.zimbra@redhat.com>
	<5adc2737-1f11-27b1-eb46-733fdd9ab9df@redhat.com>
Message-ID: <4ef7ef35-a42e-a779-5c9e-e5f411c230e8@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

Hi,


On 11/12/17 11:53, Andrew Price wrote:
> On 08/12/17 15:30, Bob Peterson wrote:
>> Hi,
>>
>> This is a version 2 replacement for this patch, with some improvements.
>> ---
>> This patch just adds the capability for GFS2 to track which function
>> called gfs2_log_flush. This should make it easier to diagnose
>> problems based on the sequence of events found in the journals.
>>
>> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> <snip>
>> --- a/include/uapi/linux/gfs2_ondisk.h
>> +++ b/include/uapi/linux/gfs2_ondisk.h
>> @@ -406,6 +406,42 @@ struct gfs2_log_header {
>> ????? __be32 lh_hash;
>> ? };
>> ? +/*
>> + * Log Header version 2 constants - actor who wrote a log header
>> + */
>> +
>> +enum gfs2_log_flushers {
>> +??? /* Constants reserved for kernel space */
>> +??? LHF_SHUTDOWN??????? = 0,
>
> Should 0 be used for "unknown"?
>
Yes
>> +??? LHF_JDATA_WPAGES??? = 1,
>> +??? LHF_SET_FLAGS??????? = 2,
>> +??? LHF_AIL_EMPTY_GL??? = 3,
>> +??? LHF_AIL_FLUSH??????? = 4,
>> +??? LHF_RGRP_GO_SYNC??? = 5,
>> +??? LHF_INODE_GO_SYNC??? = 6,
>> +??? LHF_INODE_GO_INVAL??? = 7,
>> +??? LHF_FREEZE_GO_SYNC??? = 8,
>> +??? LHF_KILL_SB??????? = 9,
>> +??? LHF_DO_SYNC??????? = 10,
>> +??? LHF_INPLACE_RESERVE??? = 11,
>> +??? LHF_WRITE_INODE??????? = 12,
>> +??? LHF_MAKE_FS_RO??????? = 13,
>> +??? LHF_SYNC_FS??????? = 14,
>> +??? LHF_EVICT_INODE??????? = 15,
>> +??? LHF_TRANS_END??????? = 16,
>> +??? LHF_LOGD_JFLUSH_REQD??? = 17,
>> +??? LHF_LOGD_AIL_FLUSH_REQD??? = 18,
>> +??? LHF_LOG_FLUSHERS??? = 19, /* number of kernel log flushers */
>> +
>> +??? /* Constants reserved for user space / gfs2-utils */
>> +??? LHF_GFS2_CONVERT?????????? = 26,
>> +??? LHF_GFS2_EDIT??????? = 27,
>> +??? LHF_GFS2_FSCK??????? = 28,
>> +??? LHF_GFS2_FSCK_JREPLAY??? = 29,
>> +??? LHF_GFS2_MKFS??????? = 30,
>> +??? LHF_GFS2_JADD??????? = 31
>
> Would it be better to have values for the purposes that the utils 
> would need to touch the log header, e.g. LHF_UTIL_REPLAY, LHF_UTIL_FIX 
> or LHF_UTIL_CREATE. That way, if a util is renamed or a new util is 
> written (even some third party tool that we don't know about) they 
> would still have a sensible value to use.
>
> Cheers,
> Andy
>
Yes. Also lets not make this an enum. It would make more sense to have 
it as a set of flags. That way we can have a single field but containing 
two different meanings. One set would give us the reason for the log 
header (normal log flush, recovery, freeze, or whatever) and another 
single bit flag telling us whether it is userspace or kernel space. We 
can then have an origin, which can be numbered independently in 
userspace and kernel space which indicates which function it was called 
from.

That also gets rid of that big log_flush_types array. So a typical call 
to gfs2_log_flush() would look like:

gfs2_log_flush(sdp, NULL, GFS2_LF_SYNC|GFS2_LO_KILLSB);

in this case assuming it is called from gfs2_kill_sb(). The flag for the 
kernel would be added automatically in log_write_header(), since it 
would be needed for everything. That would make the code rather easier 
to understand, since the two independent fields would be declared 
separately. There are 32 bits to use, so no issue to divide it into two 
16 bit sub fields for example,

Steve.