Writing good commit comments is critical to ensuring that changes are easily understood, even years after they were originally written. The commit comment should contain enough information about the change to allow the reader to understand the motivation for the change, what parts of the code it is affecting, and any interesting, unusual, or complex parts of the change to draw attention to.
The reason for a change may be manyfold: bug, enhancement, feature, code style, etc. so providing information about this sets the stage for understanding the change. If it is a bug, include information about what usage triggers the bug and how it manifests (error messages, LBUG, etc). If it is a feature, include information about what improvement is being made, and how it will affect usage.
Providing some high-level information about the code path that is being modified is useful for the reader, since the files and patch fragments are not necessarily going to be listed in a sensible order in the patch. Including the important functions being modified provides a starting point for the reader to follow the logic of the change, and makes it easier to search for such changes in the future.
If the patch is based on some earlier patch, then including the git commit hash of the original patch, Jira ticket number, etc. is useful for tracking the chain of dependencies. This can be very useful if a patch is landed separately to different maintenance branches, if it is fixing a problem in a previously-landed patch, or if it is being imported from an upstream kernel commit.
Having long commit comments that describe the change well is a good thing. The commit comments will be tightly associated with the code for a long time into the future, even many of the original commit comments from years earlier are still available through changes of the source code repository. In contrast, bug tracking systems come and go, and cannot be relied upon to track information about a change for extended periods of time.
Unlike the content of the commit message, the format is relatively easy to verify for correctness. Having the same standard format allows Git tools like git shortlog to extract information from the patches more easily.
The first line of the commit comment is the commit summary of the change. Changes submitted to the fs/lustre-release branch also require a Lustre Jira issue tag at the beginning of the first line of the commit summary. A Lustre Jira ticket is one that begins with LU and is therefore part of the Lustre project within Jira. If the patch is submitted for the fs/lustre-release repository without a Lustre Jira ID in the first line, then it will automatically receive a -2 review which will prevent the patch from being submitted to a release branch. You will get an e-mail about this rejection at which time you can add the line and resubmit the patch. The commit summary should also have a component: field immediately following the Jira issue tag that indicates which Lustre subsystem that the commit is related to. Example Lustre subsystems relate to modules like: llite, lov, osc, mdc, lmv, ldlm, ptlrpc, mds, oss, mdd, osd, ldiskfs, lnet, libcfs, socklnd, o2iblnd; functional components like recovery, quota, grant; or auxiliary components like build, tests, iokit, docs. This subsystem list is not exhaustive, but provides a good guideline for consistency.
The rest of the comments should be wrapped to 70 columns or less. This allows for the first line to be used a subject in emails, and also for the entire body to be displayed using tools like git log or git shortlog in an 80 column window.
LU-nnn component: short description of change under 64 columns The "component:" should be a lower-case single-word subsystem of the Lustre code that best encompasses the change being made. Examples of components include modules like: llite, lov, lmv, osc, mdc, ldlm, lnet, ptlrpc, mds, oss, osd, ldiskfs, libcfs, socklnd, o2iblnd; functional subsystems like: recovery, quota, grant; and auxilliary areas like: build, tests, docs. This list is not exhaustive, but is a guideline. The commit comment should contain a detailed explanation of the change being made. This can be as long as you'd like. Please give details of what problem was solved (including error messages or problems that were seen), a good high-level description of how it was solved, and which parts of the code were changed (including important functions that were changed, if this is useful to understand the patch, and for easier searching). Wrap lines at/under 70 columns. Signed-off-by: Your Real Name <your_email@domain.name> Change-Id: Ixxxx(added automatically if missing)xxxx |
Gerrit will automatically identify updates to existing patches and update the existing request instead of creating a new one. This preserves the history of patch comments, and allows comparing old and new versions of a patch for more efficient inspections. For this to work properly the changes you create locally need to have a unique commit id in them.
Please make sure the Change-Id: line and Signed-off-by: lines are at the bottom of the comment. This is required by the fs/lustre-release project (at least) in order for the patch to be approved. If your comments are missing either of these lines the patch will be automatically be rejected at submission time.
To get the Commit-Id: field inserted automatically, link the build/commit-msg hook into each Git repository's .git/hooks/ directory. This will update the commit message for changes you commit to your repository before they are submitted to Gerrit. The commit-msg hook will also verify that the commit message format matches the format specified above. Similarly, linking the build/prepare-commit-msg script into .git/hooks/ will run the build/checkpatch.pl script against each patch at commit time to check it against the Lustre Coding Guidelines. From the top-level Lustre tree checkout:
ln -sf ../../build/commit-msg .git/hooks/ ln -sf ../../build/prepare-commit-msg .git/hooks/ |
For older Lustre versions that are missing these commit hooks, they can be copied from a newer branch of Lustre (e.g. master).
A number of additional commit tags can be used to further explain who has contributed to the patch, and for tracking purposes. These tags are commonly used with Linux kernel patches. These tags should appear before the Signed-off-by: tag.
Acked-by: User Name <user@domain.com>
Tested-by: User Name <user@domain.com>
Reported-by: User Name <user@domain.com>
Reviewed-by: User Name <user@domain.com>
CC: User Name <user@domain.com>
{Organization}-bug-id: Arbitrary identifier
|
The {Organization}-bug-id: tag can be used to track this patch in other bug databases.
|
LU-477 ldiskfs: allocate s_group_desc/s_group_info by vmalloc() Add the patch to the RHEL6 ldiskfs patch series. Large kmalloc() for sbi->s_group_desc and sbi->s_group_info can fail for large filesystems (typically over 16TB), which will cause the "not enough memory" error while mounting. This patch makes allocation fall back to vmalloc() if the kmalloc() failed, as what was done for sbi->s_flex_groups. To avoid colliding with an valid on-disk inode number, EXT4_BAD_INO is used as the number of the buddy cache inode. The patch also incorporates the following upstream kernel fix: commit 32a9bb57d7c1fd04ae0f72b8f671501f000a0e9f ext4: fix missing iput() of root inode for some mount error paths https://bugzilla.kernel.org/show_bug.cgi?id=26752 Signed-off-by: Yu Jian <yujian whamcloud.com> Change-Id: I3950425835ea7f2968ceb2edbc622e3ff3ed8545 |
LU-723 build: Enhance lustre/ldiskfs build system Enhance the lustre/ldiskfs build system so it is more robust, flexible, and consistent with lustre/zfs build system. This change is being made in the interest of standardizing the infra-structure around backend filesystems. This change does not effect the current behavior of the --with-ldiskfs, --enable-ext4, or --with-ldiskfsprogs configure options. However, it does remove the obsolete --with-ldiskfs-inkernel configure option which was only used by LLNL. It also adds the --with-ldiskfs-obj configure option which improves flexibility. And the --enable-ldiskfs-build configure option to support building against the lustre-ldiskfs-devel package. The behavior of these options is consistent with their ZFS counterparts, see commit 8c7266c for further details. --enable-ext4 enable ldiskfs build using ext4 --enable-ldiskfs-build enable ldiskfs configure/make --with-ldiskfs=path set path to ldiskfs source --with-ldiskfs-obj=path set path to ldiskfs objects --with-ldiskfsprogs use alternate names for ldiskfs-enabled e2fsprogs Sample ./configure results when building lustre and ldiskfs using the kernel-devel and kernel-debuginfo-common packages. checking whether to enable ldiskfs... yes checking ldiskfs source directory... /home/behlendo/src/git/lustre/ldiskfs checking ldiskfs object directory... /home/behlendo/src/git/lustre/ldiskfs checking ldiskfs module symbols... Module.symvers checking ldiskfs source release... 3.3.0 checking whether to use ext3 or ext4 source... ext4 checking ext4 source directory... /usr/src/debug/.../fs/ext4 checking whether to build ldiskfs... yes checking for /home/behlendo/src/git/lustre/ldiskfs/configure... yes checking for /usr/src/debug/.../fs/ext4/dir.c... yes checking for /usr/src/debug/.../fs/ext4/file.c... yes checking for /usr/src/debug/.../fs/ext4/inode.c... yes checking for /usr/src/debug/.../fs/ext4/super.c... yes checking if ext4_ext_walk_space() takes i_data_sem... yes checking if LDISKFS_SINGLEDATA_TRANS_BLOCKS takes sb as argument... yes checking if ldiskfs_discard_preallocations defined... yes checking if ldiskfs_ext_insert_extent needs 5 arguments... yes In the context of this change additional cleanup has been done and all of the ldiskfs specific code relocated to lustre-build-ldiskfs.m4. Note that this change moves us closer to supporting patchless Lustre servers with ldiskfs. Once the remaining kernel patches for Lustre are dropped you will be able to build Lustre using the distribution provided kernel-devel and kernel-debuginfo-common packages. This change also incorperates ORI-340, commit f604951, which ensures that the Module.symvers file will cleanly include the symbols for all enabled Lustre backends. While the only backend supported by master right now is ldiskfs this brings the master and orion branchs in to sync in this regard. Change-Id: I6f13f266944ec6967f4d7705a30b83ab8e577b15 Signed-off-by: Brian Behlendorf <behlendorf1 llnl.gov> Signed-off-by: Prakash Surya <surya1 llnl.gov> |