Re: [PATCH v3 00/17] nfsd: refactor nfs4_create_file()

On Mon, 2026-07-13 at 16:15 +1000, NeilBrown wrote:
> This v2 incorporates fixes for issues Chuck reported from "Codex
> review".  These changes are distributed around the patches a bit, but
> 02/17 is an interesting new bugfix.
> 
> This is against nfs-testing (5f5dc3d855cf).
> 
> Thanks,
> NeilBrown
> 
>  [PATCH v3 01/17] nfsd: honour client-provided attributes for
>  [PATCH v3 02/17] nfsd: correctly handle CREATE of mounted-on files
>  [PATCH v3 03/17] nfsd: replace fh_fill_both_attrs() with
>  [PATCH v3 04/17] nfsd: move fh_want_write() after preamble in
>  [PATCH v3 05/17] nfsd: move more nfs-specific code into preamble of
>  [PATCH v3 06/17] nfsd: remove subtlety from nfsd4_create_file()
>  [PATCH v3 07/17] nfsd: in nfsd4_create_file() let VFS report if file
>  [PATCH v3 08/17] nfsd: nfsd4_create_file(): Move NFSD_MAY_CREATE
>  [PATCH v3 09/17] nfsd: fh_want_write) failure need not be immediately
>  [PATCH v3 10/17] nfsd: (almost) always open file in
>  [PATCH v3 11/17] nfsd: reduce range of directory lock in
>  [PATCH v3 12/17] nfsd: open-code nfsd4_vfs_create() into
>  [PATCH v3 13/17] nfsd: move some code out of the
>  [PATCH v3 14/17] nfsd: reduce want-write range in nfsd4_create_file()
>  [PATCH v3 15/17] nfsd: move v0 checking out of nfsd_check_obj_isreg()
>  [PATCH v3 16/17] nfsd: separate out VFS-specific code from
>  [PATCH v3 17/17] nfsd: use do_lookup_open() for non-creating open

I ran a review pass over this with an LLM and it found a few issues.
Let me know if you want the more detailed inline reviews:

Regressions found
-----------------
[high] 850097d914d3 "always open file in nfsd4_create_file()"
  dentry_open() now runs on any positive dentry the client name resolves
  to, before the nfsd_check_obj_isreg() type check in do_open_lookup().
  For an existing fifo, the default O_RDONLY (no O_NONBLOCK) open blocks in
  fifo_open() waiting for a writer, and does so while the parent directory
  i_rwsem is held (start_creating..end_creating) - an nfsd thread stall
  with the directory locked. Device nodes invoke the driver ->open. The
  ordering persists to the series head (do_lookup_open). Detail:
  series-review/850097d914d3/review-inline.txt.

[medium] 90a149d88abc "reduce want-write range in nfsd4_create_file()"
  Issue 1: For a write-mode create the setattr now runs without
  sb_start_write() freeze protection. The reduced want_write is dropped
  after end_creating(); only the O_RDONLY branch reacquires fh_want_write()
  around nfsd_create_setattr(). A write-mode open holds the mnt writer
  reference but not freeze protection, and nfsd_setattr() does not take its
  own (resfhp->fh_dentry is set, so get_write_count is false). A concurrent
  freeze_super() can complete in the window, after which notify_change()
  modifies a frozen filesystem.
  Issue 2 (low): commit message says "we add a call to dentry_open()" for
  the already-exists case, but that call pre-exists; only the comment is
  reworded. Detail: series-review/90a149d88abc/review-inline.txt.

[medium] f467dd742c2d "separate out VFS-specific ... do_lookup_open()"
  Issue 1: "if (status == NFSERR_NOENT && create_status)" compares a __be32
  status against the raw host enum NFSERR_NOENT (2). status holds
  nfserr_noent == cpu_to_be32(2) == 0x02000000 on little-endian, so the
  branch never fires and the create-permission error (create_status) is
  masked by NFS4ERR_NOENT. Should be nfserr_noent. Persists to commit 14.
  Issue 2 (trivial): duplicated word in the subject ("from from").
  Detail: series-review/f467dd742c2d/review-inline.txt.

[high] b4f1a9d54d0f "use do_lookup_open() for non-creating open requests too."
  Issue 1: do_lookup_open() begins with an unconditional mnt_want_write().
  Now that non-creating OPEN is routed through it (previously nfsd_lookup(),
  which needs no write access), a plain read-only OPEN of an existing file
  returns NFS4ERR_ROFS when the exported filesystem is mounted read-only or
  has a read-only superblock (squashfs, iso9660, erofs, ro-mounted
  ext4/xfs). mnt_want_write() also calls sb_start_write(), so a read OPEN
  now blocks while the filesystem is frozen.
  Issue 2 (medium/perf): do_lookup_open() uses start_creating(), taking the
  parent i_rwsem exclusively for every non-creating OPEN; nfsd_lookup()
  previously used lookup_one_unlocked() (shared, or none on a dcache hit),
  so concurrent opens in one directory are now serialised.
  Detail: series-review/b4f1a9d54d0f/review-inline.txt.

-- 
Jeff Layton <[email protected]>