[PATCH] jbd2: Set the right uuid for block tag

[PATCH] jbd2: Set the right uuid for block tag

[Wang Jianjian]

journal->j_uuid is not initialized and let us use the uuid from
j_superblock. And since this is the only place where j_uuid is used
so that we can remove it.

Signed-off-by: Wang Jianjian <wangjianjian0@foxmail.com>
---
 fs/jbd2/commit.c     |  2 +-
 include/linux/jbd2.h | 10 ----------
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 5b9408e3b370..efde9c494e7a 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -720,7 +720,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		bufs++;
 
 		if (first_tag) {
-			memcpy (tagp, journal->j_uuid, 16);
+			memcpy (tagp, journal->j_superblock->s_uuid, 16);
 			tagp += 16;
 			space_left -= 16;
 			first_tag = 0;
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index de9536680b2b..9d51f4b55cb5 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1079,16 +1079,6 @@ struct journal_s
 	 */
 	tid_t			j_commit_request;
 
-	/**
-	 * @j_uuid:
-	 *
-	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
-	 * backed by this journal.  This will eventually be replaced by an array
-	 * of uuids, allowing us to index multiple devices within a single
-	 * journal and to perform atomic updates across them.
-	 */
-	__u8			j_uuid[16];
-
 	/**
 	 * @j_task: Pointer to the current commit thread for this journal.
 	 */
-- 
2.34.3

Re: [PATCH] jbd2: Set the right uuid for block tag

[Wang Jianjian]

Hi all,
Is this a real problem need to fix ?

On 7/12/22 00:26, Wang Jianjian wrote:
> journal->j_uuid is not initialized and let us use the uuid from
> j_superblock. And since this is the only place where j_uuid is used
> so that we can remove it.
> 
> Signed-off-by: Wang Jianjian <wangjianjian0@foxmail.com>
> ---
>   fs/jbd2/commit.c     |  2 +-
>   include/linux/jbd2.h | 10 ----------
>   2 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 5b9408e3b370..efde9c494e7a 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -720,7 +720,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>   		bufs++;
>   
>   		if (first_tag) {
> -			memcpy (tagp, journal->j_uuid, 16);
> +			memcpy (tagp, journal->j_superblock->s_uuid, 16);
>   			tagp += 16;
>   			space_left -= 16;
>   			first_tag = 0;
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index de9536680b2b..9d51f4b55cb5 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1079,16 +1079,6 @@ struct journal_s
>   	 */
>   	tid_t			j_commit_request;
>   
> -	/**
> -	 * @j_uuid:
> -	 *
> -	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
> -	 * backed by this journal.  This will eventually be replaced by an array
> -	 * of uuids, allowing us to index multiple devices within a single
> -	 * journal and to perform atomic updates across them.
> -	 */
> -	__u8			j_uuid[16];
> -
>   	/**
>   	 * @j_task: Pointer to the current commit thread for this journal.
>   	 */
> 

Re: [PATCH] jbd2: Set the right uuid for block tag

[Theodore Ts'o]

On Fri, Jul 15, 2022 at 11:11:23PM +0800, Wang Jianjian wrote:
> Hi all,
> Is this a real problem need to fix ?
> 
> On 7/12/22 00:26, Wang Jianjian wrote:
> > journal->j_uuid is not initialized and let us use the uuid from
> > j_superblock. And since this is the only place where j_uuid is used
> > so that we can remove it.

There's a really long story, and the short version is, we don't really
need to do anything.

The longer version of the story is that journal->j_uuid is not
supposed to be the uuid from the journal superblock, but rather the
uuid from the file system.  Quoting from the jbd2.h header file:

	/**
	 * @j_uuid:
	 *
	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
	 * backed by this journal.  This will eventually be replaced by an array
	 * of uuids, allowing us to index multiple devices within a single
	 * journal and to perform atomic updates across them.
	 */
	__u8			j_uuid[16];

The original design goal from Stephen Tweedie, who implemented the jbd
subsystem for ext3, was that the jbd/jbd2 layer could in theory be
used for more than just ext3/ext4 (and indeed, it is used by
ocfs/ocfs2), *and*, that in the case of a journal on an external
device, that a single jbd/jbd2 journal could support multiple file
systems.  So you might have a dozen HDD's in a NAS box, and they would
all use a single extrenal device as a journal.

So at the beginning of each block (or revoke) tag, there is a space
for a UUID to indicate what file system that the metadata blocks being
journaled was for.  Those bits are skipped if the JBD2_FLAG_SAME_UUID
is set, in which case the tag is assumed to belong to the same file
system as the previous tag.

The on-disk space has been reserved for this design; we have
JBD2_FLAG_SAME_UUID, and that's why we copy the j_uuid into the tag
block for the first tag, and all other tags gave the SAME_UUID flag
set.  In addition, in the on-disk superblock, we define an array of
UUID's which are the file systems which use this particular external
journal:

/* 0x0100 */
	__u8	s_users[16*48];		/* ids of all fs'es sharing the log */

However, full support for having multiple file systems sharing the
long was never realized.  Part of the reason for this is there are
complications if one of the disks is off-line at the time that the
journal is replayed.  Suppose out of the dozen file systems sharing an
external journal, one of the HDD's is temporarily off-line, or removed
from the NAS box.  Now we can't replay the journal, since there is no
place to put the journal enrties for the missing file system(s).  In
theory we could copy the journal entries for the missing file system
in an file, but then we would have to define some place on the root
file system where the saved journals for the missing HDD could be
found, and that assumes that the root file system is mounted
read/write so it's available to save the journal information.

Another potential problem is that when we *do* need to do a commit, we
have to wait for handles for *all* of the file systems using that
journal to complete, and so an fsync() triggered on one file system
would potentially hold up file system operations on a sibling device.

There might be cases where this would make sense, they would be pretty
specialized situations, and so never has ever decided to implement the
rest of the feature.

At the moment journal->j_uuid is all zeros after
journal_init_common(), and so we're wasting 16 bytes in each journal
descriptor block by filling in those bytes with all zeroes.  The
proper way to "fix" this would be in fs/ext4/super.c, in the functions
ext4_get_inode() and ext4_get_dev_journal(), after those functions
call jbd2_journal_init_{dev,inode}, they should copy
EXT4_SB(super)->s_uuid into journal->j_uuid.

That would properly "initialize" journal->j_uuid, and it would mean
that the file system uuid would be in the tag block.  One potential
gotcha if we were to make this change, and if we wanted to actually
check the uuid in the jbd2 descriptor block, is that today, tune2fs,
and the proposed EXT4_IOC_SETUUID ioctl assume that we can change the
file system uuid without having any potential problems.

If we wanted to properly support this case in the EXT4_IOC_SETUUID
patch, we would have to freeze the file system by calling
ext4_freeze() and then update journal->j_uuid, as well as update
jsb->s_users[] if we are using an external journal.

And for the existing userspace-only "tune2fs -U" code, we currently
don't have a way of triggering an update of the journal->j_uuid field
when the file system uuid is changed.  (We do update the journal
superblock s_users array, but if we did that while the file system was
mounted, it wouldn't be safe unless it called FIFREEZE/FITHAW, which
it currently doesn't do.)

So we could initialize journal->j_uuid if we wanted to, which wouldn't
do much harm, but it wouldn't really fix anything either --- and then
we'd have to make sure that journal is quicesed, and journal->j_uuid
gets updated if the file system UUID is changed while the file system
is mounted.  Once the first tag in the jbd2 descriptor block is now
set correctly, we would then have to find a profitable way to *use*
that information --- either as an additional sanity check on top of
the checksum, or to implement the full support for shared uses for the
journal.  But since the journal checksums are good enough that we
don't need the additional validation, and there hasn't been a real
demand for shared external journals, it probably won't happen until
someone wants to make a business case and fund the engineering work to
make it happen.

Cheers,

						- Ted
						

Re: [PATCH] jbd2: Set the right uuid for block tag

[Wang Jianjian]

Gotcha, Thanks for your detailed explanation!

On 7/16/22 01:43, Theodore Ts'o wrote:
> On Fri, Jul 15, 2022 at 11:11:23PM +0800, Wang Jianjian wrote:
>> Hi all,
>> Is this a real problem need to fix ?
>>
>> On 7/12/22 00:26, Wang Jianjian wrote:
>>> journal->j_uuid is not initialized and let us use the uuid from
>>> j_superblock. And since this is the only place where j_uuid is used
>>> so that we can remove it.
> 
> There's a really long story, and the short version is, we don't really
> need to do anything.
> 
> The longer version of the story is that journal->j_uuid is not
> supposed to be the uuid from the journal superblock, but rather the
> uuid from the file system.  Quoting from the jbd2.h header file:
> 
> 	/**
> 	 * @j_uuid:
> 	 *
> 	 * Journal uuid: identifies the object (filesystem, LVM volume etc)
> 	 * backed by this journal.  This will eventually be replaced by an array
> 	 * of uuids, allowing us to index multiple devices within a single
> 	 * journal and to perform atomic updates across them.
> 	 */
> 	__u8			j_uuid[16];
> 
> The original design goal from Stephen Tweedie, who implemented the jbd
> subsystem for ext3, was that the jbd/jbd2 layer could in theory be
> used for more than just ext3/ext4 (and indeed, it is used by
> ocfs/ocfs2), *and*, that in the case of a journal on an external
> device, that a single jbd/jbd2 journal could support multiple file
> systems.  So you might have a dozen HDD's in a NAS box, and they would
> all use a single extrenal device as a journal.
> 
> So at the beginning of each block (or revoke) tag, there is a space
> for a UUID to indicate what file system that the metadata blocks being
> journaled was for.  Those bits are skipped if the JBD2_FLAG_SAME_UUID
> is set, in which case the tag is assumed to belong to the same file
> system as the previous tag.
> 
> The on-disk space has been reserved for this design; we have
> JBD2_FLAG_SAME_UUID, and that's why we copy the j_uuid into the tag
> block for the first tag, and all other tags gave the SAME_UUID flag
> set.  In addition, in the on-disk superblock, we define an array of
> UUID's which are the file systems which use this particular external
> journal:
> 
> /* 0x0100 */
> 	__u8	s_users[16*48];		/* ids of all fs'es sharing the log */
> 
> However, full support for having multiple file systems sharing the
> long was never realized.  Part of the reason for this is there are
> complications if one of the disks is off-line at the time that the
> journal is replayed.  Suppose out of the dozen file systems sharing an
> external journal, one of the HDD's is temporarily off-line, or removed
> from the NAS box.  Now we can't replay the journal, since there is no
> place to put the journal enrties for the missing file system(s).  In
> theory we could copy the journal entries for the missing file system
> in an file, but then we would have to define some place on the root
> file system where the saved journals for the missing HDD could be
> found, and that assumes that the root file system is mounted
> read/write so it's available to save the journal information.
> 
> Another potential problem is that when we *do* need to do a commit, we
> have to wait for handles for *all* of the file systems using that
> journal to complete, and so an fsync() triggered on one file system
> would potentially hold up file system operations on a sibling device.
> 
> There might be cases where this would make sense, they would be pretty
> specialized situations, and so never has ever decided to implement the
> rest of the feature.
> 
> At the moment journal->j_uuid is all zeros after
> journal_init_common(), and so we're wasting 16 bytes in each journal
> descriptor block by filling in those bytes with all zeroes.  The
> proper way to "fix" this would be in fs/ext4/super.c, in the functions
> ext4_get_inode() and ext4_get_dev_journal(), after those functions
> call jbd2_journal_init_{dev,inode}, they should copy
> EXT4_SB(super)->s_uuid into journal->j_uuid.
> 
> That would properly "initialize" journal->j_uuid, and it would mean
> that the file system uuid would be in the tag block.  One potential
> gotcha if we were to make this change, and if we wanted to actually
> check the uuid in the jbd2 descriptor block, is that today, tune2fs,
> and the proposed EXT4_IOC_SETUUID ioctl assume that we can change the
> file system uuid without having any potential problems.
> 
> If we wanted to properly support this case in the EXT4_IOC_SETUUID
> patch, we would have to freeze the file system by calling
> ext4_freeze() and then update journal->j_uuid, as well as update
> jsb->s_users[] if we are using an external journal.
> 
> And for the existing userspace-only "tune2fs -U" code, we currently
> don't have a way of triggering an update of the journal->j_uuid field
> when the file system uuid is changed.  (We do update the journal
> superblock s_users array, but if we did that while the file system was
> mounted, it wouldn't be safe unless it called FIFREEZE/FITHAW, which
> it currently doesn't do.)
> 
> So we could initialize journal->j_uuid if we wanted to, which wouldn't
> do much harm, but it wouldn't really fix anything either --- and then
> we'd have to make sure that journal is quicesed, and journal->j_uuid
> gets updated if the file system UUID is changed while the file system
> is mounted.  Once the first tag in the jbd2 descriptor block is now
> set correctly, we would then have to find a profitable way to *use*
> that information --- either as an additional sanity check on top of
> the checksum, or to implement the full support for shared uses for the
> journal.  But since the journal checksums are good enough that we
> don't need the additional validation, and there hasn't been a real
> demand for shared external journals, it probably won't happen until
> someone wants to make a business case and fund the engineering work to
> make it happen.
> 
> Cheers,
> 
> 						- Ted
> 						
>