Josef “Jeff” Sipek

Looking up Files, Part II

So, here’s more updates about my adventures within the realm of unionfs_lookup (I suggest you read part I first). After my first post about lookup code, I went back to coding, and I had the pleasure to try to figure out why I was hitting a BUG_ON() with my new code, but not with the old code.

I made a simple test case, in one terminal I’d run fsx (a POSIX compliance tester program) on unionfs:

mount -t unionfs -o dirs=/mnt/foo/b0:/mnt/foo/b1=ro none unionfs/
cd unionfs/
fsx -l 104060000 -q foo

And then mid-way through, I’d insert a branch as the new branch index 0:

mount -o remount,add=/mnt/foo/b0:/mnt/foo/b2=rw /mnt/unionfs

The remount command immediatelly caused the BUG_ON (that tests for dentry validity) in unionfs_setattr to trigger. It seemed rather odd that the lookup code replacement would do something that’d cause the unionfs dentry to be invalid. I pondered for a bit, and then I tried to insert a number of branches quickly with the old code. Eureka! The same BUG_ON() got triggered. Some lxr-ing later, it became apparent that we need to potentially revalidate inside the inode ops (like unionfs_setattr). Seems kinda obvious now, oh well. I’m also pondering about the posibility of changing the VFS to call d_revalidate, but I’m still not sure if that’s the Right Thing(tm) to do.

Until next time!

Looking up Files

So, I spend the last two to three days mucking around with unionfs_lookup. Before I touched it, it was a very, very ugly, 340 line beast that no one on the Unionfs team wanted to touch in the past year or so, because it seemed that just looking at it the wrong way would make it not work. The function had 4 different modes of operation, which overlapped in subtle ways.

Well, I decided to have some fun — rewriting it from scratch. :) Currently, the lookup function is 210 lines long, and it looks like it is working. It has only one mode - as it should. Since, I am still not done, the original lookup code is still there, and used for the other 3 modes. I’ll hack on it some more, and either remove them completely, or collapse some of them because they seem a bit redundant.

In the end, even if the total code size is still around 340 lines, I’ll be happy. Having 340 lines of readable code is way better than 340 lines of barely readable code.

OLS 2007!

So, I submitted a proposal for OLS 2007, and I got in! Stay tuned for the self-inflicted agony associated with actually writing the paper :)

Step 1: Fame

Totally awesome day! I submitted Unionfs to the usual places (linux-kernel, fsdevel, and the key people), then I stayed up all night. In the morning, I got a form for permission to enroll in the graduate version of compilers (I’d much prefer lex & yacc to some made up java thing the undergrad course uses). At around 10, I decided to head home and get some sleep. I woke up about 8 hours later, and checked my email. I replied to a lot of comments/questions by Andrew Morton and some other people, and when I finally managed to check the rest of the inbox, I saw:

Jan 08 akpm@osdl.org   ( 236) + unionfs-documentation.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 107) + lookup_one_len_nd-lookup_one_len-with-nameidata-argument.patch added to -mm
Jan 08 akpm@osdl.org   ( 138) + unionfs-branch-management-functionality.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 649) + unionfs-common-file-operations.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 733) + unionfs-copyup-functionality.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 299) + unionfs-dentry-operations.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 313) + unionfs-file-operations.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 319) + unionfs-directory-file-operations.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 326) + unionfs-directory-manipulation-helper-functions.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 995) + unionfs-inode-operations.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 572) + unionfs-lookup-helper-functions.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 743) + unionfs-main-module-functions.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 344) + unionfs-readdir-state.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 501) + unionfs-rename.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 263) + unionfs-privileged-operations-workqueue.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 168) + unionfs-handling-of-stale-inodes.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 228) + unionfs-miscellaneous-helper-functions.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 402) + unionfs-superblock-operations.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 233) + unionfs-helper-macros-inlines.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 552) + unionfs-internal-include-file.patch added to -mm tree
Jan 08 akpm@osdl.org   (  87) + unionfs-include-file.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 218) + unionfs-unlink.patch added to -mm tree
Jan 08 akpm@osdl.org   ( 109) + unionfs-kconfig-and-makefile.patch added to -mm tree

Unionfs is now in -mm!

If you actually look at the next -mm changelog, you only see one patch containing all of Unionfs, as Andrew decided to use the git tree that I set up (gitweb) on kernel.org.

Kernel Hacking

So, I moved my Unionfs development from a bunch of i386 boxes, to 2 x86_64 servers. They are sweet :) I still have to get used to looking in arch/x86_64/ and include/asm-x86_64/ instead of their i386 equivalents which I have used for years.

Papers...

Hrm, the ever-ending TODO list currently contains two papers I want to read. Since the TODO list doesn’t actually exist anywhere beyond my head, I figured that I’d post the links to the papers here, and maybe someone who reads this will bug me, in turn making me actually read them.

Unionfs Request For Comments

So, finally, after long time of trying to get Unionfs into the Linux kernel, we submited it to linux-kernel, fsdevel, cc’ing all the people that should be cc’d (Al Viro, Christoph Hellwig, and Andrew Morton.)

Double u, tee, ef

You can look at this post as either a sequal to a post I made few days ago, or as another sequal to a post I made about a year ago.

So, while doing some more cleanup, Dave Quigley discovered this gem in Unionfs:

for (bindex = bstart - 1; bindex >= 0; bindex--) {
        err = copyup_file(dentry->d_parent->d_inode,
                          file, bstart, bindex,
                          file->f_dentry->d_inode->i_size);

        if (!err)
                break;
        else
                continue;
}

The particular piece of interest is the if statement:

if (!err)
        break;
else
        continue;

What does it mean? If there is no error, we break out of the loop. Makes sense. If there is an error, we go to the top of the loop. Makes sense…right? Well, what happens if you reach the end of a for loop? We go to the top of the loop! See the brain damage? We are at the end of the loop, so we don’t need an explicit continue statement.

Here’s the if statement the way it should be (for completeness):

if (!err)
        break;

Oh, so simple, it almost brings a tear into one’s eye.

Bizarre Code

Think of this as a sequel to my post about a year ago.

The following peice of code is from Unionfs’s copyup_permission(). It is (or should be) a simple function that is supposed to copy the permissions from one inode to another. Well, I can’t help but remember this one slide from Greg Kroah-Hartman’s keynote this year at Ottawa Linux Symposium. Here’s the slide:

Linux is evolution, not intelligent design.

The quote definitely applies to copyup_permission(). It seems very clear that it “evolved” to something very odd. Anyway, I shall torment you no longer, here is the code:

static int copyup_permissions(struct super_block *sb,
                              struct dentry *old_hidden_dentry,
                              struct dentry *new_hidden_dentry)
{
        struct iattr newattrs;
        int err;

        print_entry_location();

        newattrs.ia_atime = old_hidden_dentry->d_inode->i_atime;
        newattrs.ia_mtime = old_hidden_dentry->d_inode->i_mtime;
        newattrs.ia_ctime = old_hidden_dentry->d_inode->i_ctime;
        newattrs.ia_valid = ATTR_CTIME | ATTR_ATIME | ATTR_MTIME |
                        ATTR_ATIME_SET | ATTR_MTIME_SET;
        /* original mode of old file */
        newattrs.ia_mode = old_hidden_dentry->d_inode->i_mode;
        newattrs.ia_gid = old_hidden_dentry->d_inode->i_gid;
        newattrs.ia_uid = old_hidden_dentry->d_inode->i_uid;
        newattrs.ia_valid |= ATTR_FORCE | ATTR_GID | ATTR_UID | ATTR_MODE;
        if (newattrs.ia_valid & ATTR_MODE) {
                newattrs.ia_mode = (newattrs.ia_mode & S_IALLUGO) |                
                        (old_hidden_dentry->d_inode->i_mode & ~S_IALLUGO);
        }

        err = notify_change(new_hidden_dentry, &newattrs);

        print_exit_status(err);
        return err;
}

It was actually Seth Arnold that noticed that the condition will ALWAYS be true because ATTR_MODE is set in the line just above it. Furthermore, if one eliminates the if statement and replaces newattr.ia_mode in the assignment with what it is set to just few lines before (right after the comment) he gets:

newattrs.ia_mode = (old_hidden_dentry->d_inode->i_mode & S_IALLUGO) |
                   (old_hidden_dentry->d_inode->i_mode & ~S_IALLUGO);

If you are up to speed with bitwise operations, you’ll realize that it can be simplified to:

newattrs.ia_mode = old_hidden_dentry->d_inode->i_mode;

Brilliant!

OLS 2006 - Day 5

The day began with an awesome presentation I gave about Unionfs. :) Shawn was recoding it, but after the presentation, he found out that the video turned out to be crap. He has audio only. I’m sure he’ll share it soon. :) I was pleasantly surprised at the number of people that use Unionfs or were interested in Unionfs.

The keynote was excelent as always. However I must say that Greg K-H made it sound like any piece of code will get into the kernel. Yeah, right :) But he did say few nice things about the status of Linux.

After the keynote, there was the GPG key signing - which I did not attend, although I wanted to. Instead we went to get some food. Food was good, we (I, Dave, Mike Halcrow, and Prof. Zadok) talked about a bunch of things ranging from MythTV and terabyte storage servers, to things like the number of ants in Texas. (Apparently, it is a lot of fun to watch termites and fire ants battle to the death. O_o )

We finished food around 19:45 which was about right to head over to the Black Thorn for the after event party. Just as last year it was quite interesting. Pretty much as soon as I got there, I noticed Peter Baudis aka. pasky - the cogito maintainer. We chatted about how git and Mercurial differ (Matt’s talk the day before came in handy :) ). I mentioned I was slowly working on a generic benchmark script that would test a number of popular SCMs including Mercurial, Subversion, and CVS. He was thrilled about the prospect of knowing exactly where git sucked compared to other SCMs - my guess is that he wants to fix it and make it better, a noble goal, but unnecessary as Mercurial already exists and why reinvent the wheel? ;) Seriously, though, I think a lot of people would benefit from knowing exactly where each SCM excels, and where each sucks. The nice thing about collaborating with the git people would be that it would make it more apparent that this wouldn’t just be yet-another-fake-test. After some time, a bunch of other Czech people poped up right next to us (people like, Pavel Machek, etc.). It was quite interesting. :)

After than I joined a converation with some Intel people. As it turns out, one of the Intel people is working on the e1000 driver — awesome piece of hardware, by the way, don’t ever buy anything other than it. :) Some time later, Jens Axboe joined the group briefly. When he said my name seemed familiar, I mentioned how I tried to implement IO priorities - and failed :) Later on, a guy from University of Toronto joined the group. He approached me earlier in the day about unionfs on clusters. We chatted about things ranging from school (undergraduate program, and grad school) to submitting kernel code to lkml. The e1000 guy said a similar thing that we should split unionfs up into a few patches, and send it off. During the event a few people still asked me about Unionfs, which felt good :)

Then, I decided that it would be fun to talk to some IRC people. I found John Levon and Seth Arnold. We sat down, and had an interesting conversation about a number of things. Since at least some of these were quite interesting, here’s a list:

  1. How can I deal with VFS and not drink vodka or other hard liquer
  2. Everybody hates CDE, even people at Sun
  3. Solaris is dead (well, they didn’t say it, but that’s the feeling I got)
  4. Brittons have some interesting sports or at least some of the expected behavior during the sport is interesting, namely:

  1. darts - you are expected to drink as you play
  2. I can’t recall the name - gigantic pool table
  3. cricket - everyone smokes "reefer" (to quote Movement, I just find this name of the substance mildly amusing) because their games sometimes take several days

After that, they kicked everyone out as it was 2:45 already. We (Seth, John, and I) went back to the hotel. There, Prof. Zadok and Chip (who arrived on Friday) were about to get up and head to the airport. :) I just went to bed.

Powered by blahgd