Dominique Martinet <asmadeus@xxxxxxxxxxxxx> wrote:
> > +static void v9fs_upload_to_server(struct netfs_io_subrequest *subreq)
> > +{
> > + struct inode *inode = subreq->rreq->inode;
> > + struct v9fs_inode __maybe_unused *v9inode = V9FS_I(inode);
>
> Any reason to have this variable assignment at all?
I'll just remove it. The __maybe_unused suppressed the warning, otherwise I'd
have removed it already.
> p9_client_write return value should always be subreq->len, but I believe
> we should use it unless err is set.
> (It's also possible for partial writes to happen, e.g. p9_client_write
> looped a few times and then failed, at which point the size returned
> would be the amount that actually got through -- we probably should do
> something with that?)
How about something like:
- int err;
+ int err, len;
trace_netfs_sreq(subreq, netfs_sreq_trace_submit);
- p9_client_write(fid, subreq->start, &subreq->io_iter, &err);
- netfs_write_subrequest_terminated(subreq, err < 0 ? err : subreq->len,
- false);
+ len = p9_client_write(fid, subreq->start, &subreq->io_iter, &err);
+ netfs_write_subrequest_terminated(subreq, len ?: err, false);
> > + total = p9_client_read(fid, subreq->start + subreq->transferred,
> > + &subreq->io_iter, &err);
>
> Just to clarify: subreq->io_iter didn't exist (or some conditions to use
> it weren't cleared) before?
Correct. It's added in the netfs-lib patches. I've provided a way to
separate the user-side iterator from the I/O-side iterator to allow the use of
a bounce buffer for the purpose of content crypto, compression or just having
to deal with RMW cycles to a larger block size on the server.
> > + if (file) {
> > + fid = file->private_data;
> > + BUG_ON(!fid);
>
> This probably should be WARN + return EINVAL like find by inode?
> It's certainly a huge problem, but we should avoid BUG if possible...
Sure. The BUG_ON() was already there, but I can turn it into a WARN+error.
> nit: not sure what's cleaner?
> Since there's a message that makes for a bit awkward if...
>
> if (WARN_ONCE(!fid, "folio expected an open fid inode->i_private=%p\n",
> rreq->inode->i_private))
> return -EINVAL;
>
> (as a side note, I'm not sure what to make of this i_private pointer
> here, but if that'll help you figure something out sure..)
Um. 9p is using i_private. But perhaps i_ino would be a better choice:
if (file) {
fid = file->private_data;
if (!fid)
goto no_fid;
p9_fid_get(fid);
} else {
fid = v9fs_fid_find_inode(rreq->inode, writing, INVALID_UID, true);
if (!fid)
goto no_fid;
}
...
no_fid:
WARN_ONCE(1, "folio expected an open fid inode->i_ino=%lx\n",
rreq->inode->i_ino);
return -EINVAL;
> This is as follow on your netfs-lib branch:
> - WARN_ON(rreq->origin == NETFS_READ_FOR_WRITE &&
> - !(fid->mode & P9_ORDWR));
> -
> - p9_fid_get(fid);
> + WARN_ON(rreq->origin == NETFS_READ_FOR_WRITE && !(fid->mode & P9_ORDWR));
>
> So the WARN_ON has been reverted back with only indentation changed;
> I guess there were patterns that were writing despite the fid not having
> been open as RDWR?
> Do you still have details about these?
The condition in the WARN_ON() here got changed. It was:
WARN_ON(writing && ...
at one point, but that caused a bunch of incorrect warning to appear because
only NETFS_READ_FOR_WRITE requires read-access as well as write-access. All
the others:
bool writing = (rreq->origin == NETFS_READ_FOR_WRITE ||
rreq->origin == NETFS_WRITEBACK ||
rreq->origin == NETFS_WRITETHROUGH ||
rreq->origin == NETFS_LAUNDER_WRITE ||
rreq->origin == NETFS_UNBUFFERED_WRITE ||
rreq->origin == NETFS_DIO_WRITE);
only require write-access.
There will be an additional one if we roll out content crypto to 9p as we may
need to do RMW cycles occasionally - but that's off to one side just for the
moment.
> If a file has been open without the write bit it might not go through,
> and it's incredibly difficult to get such users back to userspace in
> async cases (e.g. mmap flushes), so would like to understand that.
The VFS/VM should prevent writing to files that aren't open O_WRONLY or
O_RDWR, so I don't think we should be called in otherwise.
Note that I'm intending to change the way fscache is driven when we fetch
cacheable data from the server so that I can free up the PG_fscache bit and
return it to the MM folks. Instead of marking the page PG_fscache, I mark it
PG_dirty and set page->private with a special value to indicate it should only
be written to the cache - then the writepages sees that and just writes these
pages to the cache. I have a patch to do this and it seems to work, but I
need to make ceph and cifs use netfslib before I can apply it.
> > + p9_debug(P9_DEBUG_VFS, "(cached)\n");
>
> (Not a new problem so no need to address here, but having just
> "(cached)" on a split line is a bit weird.. We first compute cached or
> not as a bool and make it %s + cached ? " (cached)" : "" or
> something... I'll send a patch after this gets in to avoid conflicts)
Okay.
> > + return netfs_page_mkwrite(vmf, NULL);
>
> (I guess there's no helper that could be used directly in .page_mkwrite
> op?)
I could provide a helper that just supplies NULL as the second argument. I
think only 9p will use it, but that's fine.
David
