On Sat, Dec 12, 2020 at 09:51:04AM -0700, Jens Axboe wrote:
> struct dentry *dentry;
> @@ -3164,17 +3165,38 @@ static const char *open_last_lookups(struct nameidata *nd,
> }
>
> if (open_flag & (O_CREAT | O_TRUNC | O_WRONLY | O_RDWR)) {
> - got_write = !mnt_want_write(nd->path.mnt);
> + if (nonblock) {
> + got_write = !mnt_want_write_trylock(nd->path.mnt);
> + if (!got_write)
> + return ERR_PTR(-EAGAIN);
> + } else {
> + got_write = !mnt_want_write(nd->path.mnt);
> + }
> /*
> * do _not_ fail yet - we might not need that or fail with
> * a different error; let lookup_open() decide; we'll be
> * dropping this one anyway.
> */
Read the comment immediately after the place you are modifying. Really.
To elaborate: consider e.g. the case when /mnt/foo is a symlink to /tmp/bar,
/mnt is mounted r/o and you are asking to open /mnt/foo for write. We get
to /mnt, call open_last_lookups() to resolve the last component ("foo") in
it. And find that the sucker happens to be an absolute symlink, so we need
to jump into root and resolve "tmp/bar" staring from there. Which is what
the loop in the caller is about. Eventually we'll get to /tmp and call
open_last_lookups() to resolve "bar" there. /tmp needs to be mounted
writable; /mnt does not.
Sure, you bail out only in nonblock case, so normally the next time around
it'll go sanely. But you are making the damn thing (and it's still too
convoluted, even after a lot of massage towards sanity) harder to reason
about.
> + if (open_flag & O_CREAT) {
> + if (nonblock) {
> + if (!inode_trylock(dir->d_inode)) {
> + dentry = ERR_PTR(-EAGAIN);
> + goto drop_write;
> + }
> + } else {
> + inode_lock(dir->d_inode);
> + }
> + } else {
> + if (nonblock) {
> + if (!inode_trylock_shared(dir->d_inode)) {
> + dentry = ERR_PTR(-EAGAIN);
> + goto drop_write;
> + }
> + } else {
> + inode_lock_shared(dir->d_inode);
> + }
> + }
> dentry = lookup_open(nd, file, op, got_write);
... as well as more bloated, with no obvious benefits (take a look
at lookup_open()).
