I'll address the comments in code in a bit but wanted to reply now.
On Sun, 2008-11-16 at 18:04 -0600, Andreas Dilger wrote:
> On Nov 11, 2008 11:18 -0800, Frank Mayhar wrote:
> > +static inline int ext4_handle_valid(handle_t *handle)
> > +{
> > + if (!handle)
> > + return 0;
> My preference is to use "if (handle == NULL)", because handle is not a
> boolean.
That's actually my preference as well, but I was trying to match other
parts of the code. I'll change it.
> > + if (handle->h_transaction == NULL)
> > + return 0;
>
> Does this ever happen? Ah, is this the case where the handle is pointing
> to the ext4_sb_info()? I think I'd prefer to have a magic value here,
> so that it isn't possible to accidentally dereference a pointer that
> happens to have NULL data in it.
So some magic value that gets stuffed into the pointer? Or just a magic
pointer value that gets stuffed into 'handle'?
> > +static inline int ext4_handle_dirty_metadata(handle_t *handle,
> > + struct inode *inode, struct buffer_head *bh)
> > +{
> > + int err = 0;
> > +
> > + if (ext4_handle_valid(handle)) {
> > + err = __ext4_journal_dirty_metadata(__func__, handle, bh);
> > + } else {
> > + err = __ext4_write_dirty_metadata(inode, bh);
> > + }
>
> You don't need to initialize "err = 0" here.
Good point.
> > struct ext4_sb_info {
> > + int *s_nojournal_flag; /* Null to indicate "not a handle" */
>
> I don't really see where and how this is used.
Yeah, this is pretty opaque, I'm getting similar comments from the
internal reviewer. This is how I was distinguishing the "pointer to a
real handle" from the "pointer to ext4_sb_info".
> > @@ -97,6 +97,8 @@
> > {
>
> Please, in the future use "diff -up" so that it includes the function
> names in the patch context. Without the function names it is difficult
> to know what is being changed by the patch.
Sorry, I usually do that but forgot in this case.
> > @@ -4657,7 +4659,7 @@
> > - if (metadata) {
> > + if (ext4_handle_valid(handle) && metadata) {
>
> It is probably a tiny bit more efficient to check "metadata" first.
OK.
> > @@ -136,13 +136,16 @@
> > + if (journal) {
> > + if (is_journal_aborted(journal)) {
> > + ext4_abort(sb, __func__,
> > + "Detected aborted journal");
> > + return ERR_PTR(-EROFS);
> > + }
> > + return jbd2_journal_start(journal, nblocks);
> > }
> > + current->journal_info = (handle_t *)EXT4_SB(sb);
> > + return current->journal_info;
>
> So, this appears to be the place where ext4_sb_info->s_nojournal_flag is
> actually used. To make this more clear, it would be better to do actually
> reference this variable here so that it is easier for the reader to follow.
>
> current->journal_info = &EXT4_SB(sb)->s_nojournal_flag;
Hmm. Okay, I'll have to think about this a bit; I haven't had my coffee
yet.
> > @@ -588,7 +599,8 @@
> > }
> > #endif
> > ext4_discard_preallocations(inode);
> > + if (EXT4_SB(inode->i_sb)->s_journal)
>
> I think this is the same thing as EXT4_JOURNAL(inode)?
Yeah, I noticed this after the patch went out.
> > static void ext4_write_super(struct super_block *sb)
> > {
> > + if (EXT4_SB(sb)->s_journal) {
> > + if (mutex_trylock(&sb->s_lock) != 0)
> > + BUG();
> > + sb->s_dirt = 0;
> > + }
> > + else
> > + ext4_commit_super(sb, EXT4_SB(sb)->s_es, 1);
>
> This should be "} else { ... }"
>
> > + /*
> > + * We don't want to clear needs_recovery flag when we failed
> > + * to flush the journal.
> > + */
> > + if (jbd2_journal_flush(journal) < 0)
> > + return;
>
> This comment needs to be wrapped at 80 columns.
>
>
> I like this patch a lot better than the previous one. It remains very
> readable, and isn't too intrusive to the code.
Okay, thanks, and thanks for the comments. I'll make the indicated
changes and roll another patch today.
--
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
