[Cluster-devel] [GFS2 PATCH] GFS2: Log the reason for log flushes in every log header
Steven Whitehouse
swhiteho at redhat.com
Mon Dec 11 12:12:05 UTC 2017
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 at 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.
More information about the Cluster-devel
mailing list