Re: symlinks and NWFS

From: Alexander Viro (viro@math.psu.edu)
Date: Fri May 12 2000 - 11:07:48 EDT

  • Next message: James H. Cloos Jr.: "Re: [PATCH] Recent VM fiasco - fixed"

    On Fri, 12 May 2000, Jeff V. Merkey wrote:

    [snip]
    > look at DIR.C and see if you think I'm doing the POSIX stuff right. I

    Hmm... The latest timestamps on code I've looked at were about Apr 22 and
    I had picked it from your site tonight. If you have newer variant...

    > > 1) // if somebody already freed the directory record for this ino,
    > > // then just return.
    > > in read() sounds *wrong*. If it means what it seems to mean it
    > > breaks the semantics of unlink() - unlink() should have no effect
    > > on later read() if the file had been opened prior to it.
    > > POSIX-mandated, yodda, yodda and there is a lot of software relying
    > > on that.
    >
    > readpage() or file_read()? Is this the code in FILE.C?

    In nwfs_file_read(), nwfs_file_read_kernel(), etc. Yep, file.c.

    > > 3) nwfs_rename() looks deadlocky - you lock two objects and do not
    > > seem to care about the order of locking. And you are doing that
    > > very close to the entry, so it doesn't look like some other
    > > serialization may save you here.
    >
    > This is OK -- I am observing lock ordering here, though I admit what's
    > there should definately be cleaned up. I have fine grained locking in
    > the FS -- I need a reader/writer lock instead of a semaphore here.

    Umm... Fragment in question:
    int nwfs_rename(struct inode *oldDir, struct dentry *old_dentry,
                    struct inode *newDir, struct dentry *new_dentry)
    [snip]
        register HASH *odirhash = (HASH *) oldDir->u.generic_ip;
        register HASH *ndirhash = (HASH *) newDir->u.generic_ip;
    [snip]
        if (oldDirNo == newDirNo)
           NWLockFile(odirhash);
        else
        {
           NWLockFile(odirhash);
           NWLockFile(ndirhash);
        }
    [snip]

    where NWLockFile(foo) (without VFS_DEBUG and with LINUX_SLEEP - looks like
    your default) turns into
        if (WaitOnSemaphore((struct semaphore *)&foo->Semaphore) == -EINTR)
                    ....
    and WaitOnSemaphore(foo) is down(foo) (without DEBUG_DEADLOCKS). Even with
    DEBUG_DEADLOCKS you'll get down_interruptible(foo), which is breakable but
    locks all the same.

            Erm... OK, I could see the point if you only took that lock for
    directories that have ->i_sem or ->i_zombie taken, but then.. what's the
    point of taking it at all?

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



    This archive was generated by hypermail 2b29 : Fri May 12 2000 - 11:17:17 EDT