Re: [PATCH 06/16] GFS2: dentry, export, super and vm operations

From: Steven Whitehouse
Date: Mon Sep 04 2006 - 11:18:54 EST


Hi,

On Mon, 2006-09-04 at 11:06 +0200, Jan Engelhardt wrote:
> >+ this = &fh_obj.this;
> >+ fh_obj.imode = DT_UNKNOWN;
> >+ memset(&parent, 0, sizeof(struct gfs2_inum));
> >+
> >+ switch (fh_type) {
> >+ case 10:
> >+ parent.no_formal_ino = ((uint64_t)be32_to_cpu(fh[4])) << 32;
> >+ parent.no_formal_ino |= be32_to_cpu(fh[5]);
> >+ parent.no_addr = ((uint64_t)be32_to_cpu(fh[6])) << 32;
> >+ parent.no_addr |= be32_to_cpu(fh[7]);
> >+ fh_obj.imode = be32_to_cpu(fh[8]);
> >+ case 4:
>
> What do these constants specify? Would not it be better to have a #define or
> enum{} for them somewhere?
>
The sizes of the NFS file handles in units of sizeof(u32). I've added a
couple of #defines as requested.

>
> >+ if (IS_ERR(inode))
> >+ return ERR_PTR(PTR_ERR(inode));
>
> Just return inode.
>
The function returns a dentry, so it would need to be casted and I
thought that would look "more odd" than this construction.

> >+ if (!strncmp(sb->s_type->name, "gfs2meta", 8))
> >+ return; /* meta fs. don't do nothin' */
>
> Don't nobody go nowhere![1] You see, american negation can be fun, but well,
> just say: Nothing to do for gfs2meta.
>
> >+ sb->s_fs_info = NULL;
>
> Required?
>
Both fixed.

> >+static void gfs2_write_super(struct super_block *sb)
> >+{
> >+ struct gfs2_sbd *sdp = sb->s_fs_info;
> >+ gfs2_log_flush(sdp, NULL);
> >+}
>
> gfs2_log_flush(sb->s_fs_info, NULL)
>
ok.

> >+static void gfs2_unlockfs(struct super_block *sb)
> >+{
> >+ struct gfs2_sbd *sdp = sb->s_fs_info;
> >+ gfs2_unfreeze_fs(sdp);
> >+}
>
>
I just realised that I missed this one, but it will be in the next
patch...

>
> >+ for (x = 2;; x++) {
> >+ uint64_t space, d;
> >+ uint32_t m;
> >+
> >+ space = sdp->sd_heightsize[x - 1] * sdp->sd_inptrs;
> >+ d = space;
> >+ m = do_div(d, sdp->sd_inptrs);
>
> Note for Ingo: simple "/" division does not do it:
>
> 11:00 gwdg-wb04A:/dev/shm > cat foo.c
> #include <linux/types.h>
>
>
> uint32_t foo(uint32_t arg) {
> uint64_t bar = 1337;
> return bar / arg;
> }
>
>
> 11:00 gwdg-wb04A:/dev/shm > make -C /lib/modules/2.6.17.11-jen33-default/build
> M=$PWD modules
> make: Entering directory /usr/src/linux-2.6.17.11-jen33-obj/i386/default'
> make -C ../../../linux-2.6.17.11-jen33
> O=../linux-2.6.17.11-jen33-obj/i386/default modules
> CC [M] /dev/shm/foo.o
> Building modules, stage 2.
> MODPOST
> WARNING: "__udivdi3" [/dev/shm/foo.ko] undefined!
> CC /dev/shm/foo.mod.o
> LD [M] /dev/shm/foo.ko
> make: Leaving directory /usr/src/linux-2.6.17.11-jen33-obj/i386/default'
>
>
Yes, I'd spotted this in your email earlier on in the day, so thats why
I didn't reply to this thread before.

>
> >+/**
> >+ * gfs2_assert_warn_i - Print a message to the console if @assertion is false
> >+ * Returns: -1 if we printed something
> >+ * -2 if we didn't
> >+ */
>
> Why not just 0 and 1?
>
gfs2_assert_warn_i is called from the gfs2_assert_warn macro in util.h.
The macro evaluates to 0 in the case that the assert is not triggered,
so anything non-zero means that it was triggered, even if nothing was
printed. The patch this time is:

http://www.kernel.org/git/?p=linux/kernel/git/steve/gfs2-2.6.git;a=commitdiff;h=5acd3967347dab361d296d39ba19f8241507ef65

Steve.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/