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 6

I woke up at about 10. Took a shower, packed up and checked out. Just as last year, I decided to take my camera and wonder around Ottawa taking photos of interesting looking things. Well, the center of Ottawa is small. By about 14:00, I was kind of bored. So I decided to go back to the hotel and waste some more time. In front of the hotel, I noticed the shuttle to the airport. I asked the guy how frequently he was going, but he misunderstood me and told me how long it takes to get to the airport instead. He then asked if I was going. Since I didn’t have anything better to do, I told him that I’d just grab my bags.

We got to the airport at 14:36. So it took 15 minutes to get there from the hotel. Not bad. Then, I however realized that there wasn’t much to do at the airport. I sat down on one of the benches. Not even 10 minutes later, Movement poped up out of nowhere. We chatted about how exciting flying is. His flight back to .uk was with a layover in Atlanta - not fun. Around 15:10 he left because he was about to check in. I decided that the terminal couldn’t be any more boring, so I checked in as well. During the chat, I decided to take a photo of him. This year that’s the only OLS photo of a person! All the other photos are of Ottawa. I’ll put it up in few days, when I dig it up.

Good thing I checked in :) When I got the the terminal, I sat down near the first outlet I saw. I was about half way though the boot sequence when I notice about 7 other OLS attendees not far from me. They included Mike Halcrow, Val Henson, and some other people some of which seemed familiar, but who’s names I did not know. We chatted. Mike actually showed me a leak in the FiST templates as well as Unionfs. We chatted, until most of them left because their plane was ready. I and some other dude moved to another cluster of chairs with some more OLS attendees which appeared as we were chatting. There we had an interesting discussion about Australian politics. Some really odd things happen there. :) Some time later, they all headed to their plane (yeah, my flight was at 18:00, and I got to the airport at 14:36…). Few clusters of chairs away, I noticed Dave Jones. I decided to walk up, and ask him if he thinks there were enough changes to the bug tracking issue he was talking about in his keynote last year. He thinks that there were some, but most of them were minor. He was really hoping someone would make some automatic bug-report moving tool which automatically moved kernel bugs from say the Red Hat bugzilla to the kernel.org bugzilla.

Then they called my flight. It was the same type of a plane as on my way to Ottawa - made by Bombardier - a small jet. The flight itself was uneventful and everything went well and on time. One thing that I found interesting was the fact that the US customs people are in Ottawa and not in New York.

When I got home, I just fell asleep. :)

Powered by blahgd