Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	David Howells <dhowells@redhat.com>,
	Christoph Hellwig <hch@infradead.org>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v13] fs: Add VirtualBox guest shared folder (vboxsf) support
Date: Fri, 16 Aug 2019 00:56:54 -0700
Message-ID: <20190816075654.GA15363@infradead.org> (raw)
In-Reply-To: <20190815131253.237921-2-hdegoede@redhat.com>

A couple minor comments.  Otherwise we should be fine, things aren't
going to get much better for such a messed up protocol design.

> +			return dir_emit(ctx, d_name, strlen(d_name),
> +					fake_ino, d_type);
> +		} else {
> +			return dir_emit(ctx,
> +					info->name.string.utf8,
> +					info->name.length,
> +					fake_ino, d_type);
> +		}
> +	}

Nitpick: no need for an else after a return.

> +static int vboxsf_file_release(struct inode *inode, struct file *file)
> +{
> +	struct vboxsf_inode *sf_i = VBOXSF_I(inode);
> +	struct vboxsf_handle *sf_handle = file->private_data;
> +
> +	filemap_write_and_wait(inode->i_mapping);

Normal Linux semantics don't include writing back data on close, so
if you are doing this to follow other things like NFS CTO semantics
it should have a comment explaining that.

> +
> +	mutex_lock(&sf_i->handle_list_mutex);
> +	list_del(&sf_handle->head);
> +	mutex_unlock(&sf_i->handle_list_mutex);
> +
> +	kref_put(&sf_handle->refcount, vboxsf_handle_release);
> +	file->private_data = NULL;

There is no need to zero ->private_data on release, the file gets
freed and never reused.

> + * Ideally we would wrap generic_file_read_iter with a function which also
> + * does this check, to reduce the chance of us missing writes happening on the
> + * host side after open(). But the vboxsf stat call to the host only works on
> + * filenames, so that would require caching the filename in our
> + * file->private_data and there is no guarantee that filename will still
> + * be valid at read_iter time. So this would be in no way bulletproof.

Well, you can usually generate a file name from file->f_path.dentry.
The only odd case is opened by unliked files.  NFS has a special hack
for those called sillyrename (you can grep for that).  How similar to
normal posix semantics are expected from this fs?

> +
> +	mutex_lock(&sf_i->handle_list_mutex);
> +	list_for_each_entry(h, &sf_i->handle_list, head) {
> +		if (h->access_flags == SHFL_CF_ACCESS_WRITE ||
> +		    h->access_flags == SHFL_CF_ACCESS_READWRITE) {
> +			kref_get(&h->refcount);

Does this need a kref_get_unless_zero to deal with races during list
removal?


  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-15 13:12 Hans de Goede
2019-08-15 13:12 ` Hans de Goede
2019-08-16  7:56   ` Christoph Hellwig [this message]
2019-08-16  9:01     ` Hans de Goede
2019-08-16  9:39       ` Hans de Goede
2019-08-16 12:49       ` Christoph Hellwig
2019-08-15 13:33 ` Hans de Goede

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190816075654.GA15363@infradead.org \
    --to=hch@infradead.org \
    --cc=dhowells@redhat.com \
    --cc=hdegoede@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org linux-fsdevel@archiver.kernel.org
	public-inbox-index linux-fsdevel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox