Re: symlinks and NWFS

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


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 : Mon May 15 2000 - 21:00:20 EST