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 -- You received this message because you are subscribed to the Google Groups "linux-cachefs@xxxxxxxxxx" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-cachefs+unsubscribe@xxxxxxxxxx.