On Thu, 2008-10-30 at 17:40 -0600, Andreas Dilger wrote:
> > void ext4_dirty_inode(struct inode *inode)
> > {
> > + handle_t *current_handle = ext4_journal_current_handle(inode->i_sb);
> > + if (!current_handle) {
> > + handle_t *current_handle = ext4_journal_current_handle(inode->i_sb);
>
> Wouldn't ext4_journal_current_handle() just return NULL always for an
> unjournalled ext4 filesystem? Unfortunately, I can't see what "sb" is
> used for because your patch doesn't include the ext4_journal_current_handle()
> code.
Sorry about that, I'm doing the work in a p4 repository and somehow that
file didn't get p4 opened so it didn't generate the diff. The short
version is that it was checking the COMPAT_HAS_JOURNAL flag.
> One option is to start with a wrapper like "ext4_handle_valid(handle)"
> instead of checking "handle == NULL" everywhere. Then, we could put
> a magic value into "handle" and current->journal_info (maybe the
> the ext3_sb_info pointer). Put a magic value at the start of ext4_sb_info
> that can be validated as never belonging to a journal handle, and then you
> don't need to pass "sb" everywhere. It also allows you to distinguish
> between the "no handle was ever started" case and "running unjournalled".
Okay, yeah, this sounds like the way to go. I had seen the previous
handle==NULL case but had put it aside to get a proof-of-concept
implementation going as quickly as possible. Your explanation here
clears things up.
My suggestion is, for the non-journalling flag, set the first field
(which in the handle is a pointer to the transaction) to NULL to
distinguish it from a real handle. As far as I can tell from browsing
the code the h_transaction pointer in a real handle should never be
NULL. Please let me know if this is not the case. And maybe offer
another suggestion...?
(As an aside, this particular situation is one of the reasons a friend
of mine, Tom Van Vleck, strongly insists on putting magic numbers and
versions into structures. I'm not as insistent about it as he is but it
certainly would have helped here.)
> In any case, I'm not sure if this code is completely correct, since the
> previous code allowed calling ext4_dirty_inode() without first starting
> a journal handle, and now it would just silently do nothing and cause
> filesystem corruption for the journalled case.
So now handle==NULL will only refer to this case, correct? And I infer
from your comment that handle != NULL refers to a started handle, that
is, a handle that has a non-NULL h_transaction pointer (for my
purposes).
--
Frank Mayhar <fmayhar@xxxxxxxxxx>
Google, Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
