[bug report] pNFS/flexfiles: don't attempt pnfs on fatal DS errors

[Dan Carpenter <dan.carpenter@linaro.org>]

Hello Tigran Mkrtchyan,

Commit f06bedfa62d5 ("pNFS/flexfiles: don't attempt pnfs on fatal DS
errors") from Jun 27, 2025 (linux-next), leads to the following
Smatch static checker warning:

	fs/nfs/flexfilelayout/flexfilelayout.c:880 ff_layout_pg_init_read()
	error: uninitialized symbol 'ds_idx'.

We recently changed ff_layout_choose_ds_for_read() from returning NULL to
returning error pointers.  There are two bugs.  One error path in
ff_layout_choose_ds_for_read() accidentally returns success.  And some
of the callers are still checking for NULL instead of error pointers.

Btw, I'm always promoting my blog about error pointers and NULL:
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
It's not really related here, since we should not mix error pointers
and NULL but I still link to it because that's what they taught me in
my Trump Wealth Institute course on Search Engine Optimization.

Here is what ff_layout_choose_ds_for_read() looks like:
fs/nfs/flexfilelayout/flexfilelayout.c
   758  static struct nfs4_pnfs_ds *
   759  ff_layout_choose_ds_for_read(struct pnfs_layout_segment *lseg,
   760                               u32 start_idx, u32 *best_idx,
   761                               bool check_device)
   762  {
   763          struct nfs4_ff_layout_segment *fls = FF_LAYOUT_LSEG(lseg);
   764          struct nfs4_ff_layout_mirror *mirror;
   765          struct nfs4_pnfs_ds *ds = ERR_PTR(-EAGAIN);
   766          u32 idx;
   767  
   768          /* mirrors are initially sorted by efficiency */
   769          for (idx = start_idx; idx < fls->mirror_array_cnt; idx++) {
   770                  mirror = FF_LAYOUT_COMP(lseg, idx);
   771                  ds = nfs4_ff_layout_prepare_ds(lseg, mirror, false);
   772                  if (IS_ERR(ds))
   773                          continue;
   774  
   775                  if (check_device &&
   776                      nfs4_test_deviceid_unavailable(&mirror->mirror_ds->id_node))
   777                          continue;

Bug #1:  If we hit this continue on the last iteration through the loop
then we return success and *best_idx is not initialized.  It should be:

		if (check_device &&
		    nfs4_test_deviceid_unavailable(&mirror->mirror_ds->id_node)) {
			ds = ERR_PTR(-EINVAL);
			continue;
		}

   778  
   779                  *best_idx = idx;
   780                  break;
   781          }
   782  
   783          return ds;
   784  }

Bug #2: The whole rest of the call tree expects NULL and not error pointers.
For example, ff_layout_get_ds_for_read():

fs/nfs/flexfilelayout/flexfilelayout.c
   812  static struct nfs4_pnfs_ds *
   813  ff_layout_get_ds_for_read(struct nfs_pageio_descriptor *pgio,
   814                            u32 *best_idx)
   815  {
   816          struct pnfs_layout_segment *lseg = pgio->pg_lseg;
   817          struct nfs4_pnfs_ds *ds;
   818  
   819          ds = ff_layout_choose_best_ds_for_read(lseg, pgio->pg_mirror_idx,
   820                                                 best_idx);
   821          if (ds || !pgio->pg_mirror_idx)
                    ^^
   822                  return ds;

This needs to check for error pointers.  But also if pgio->pg_mirror_idx
is zero, is that a success return?  I don't know...

   823          return ff_layout_choose_best_ds_for_read(lseg, 0, best_idx);
   824  }

fs/nfs/flexfilelayout/flexfilelayout.c
    842 static void
    843 ff_layout_pg_init_read(struct nfs_pageio_descriptor *pgio,
    844                         struct nfs_page *req)
    845 {
    846         struct nfs_pgio_mirror *pgm;
    847         struct nfs4_ff_layout_mirror *mirror;
    848         struct nfs4_pnfs_ds *ds;
    849         u32 ds_idx;
    850 
    851         if (NFS_SERVER(pgio->pg_inode)->flags &
    852                         (NFS_MOUNT_SOFT|NFS_MOUNT_SOFTERR))
    853                 pgio->pg_maxretrans = io_maxretrans;
    854 retry:
    855         pnfs_generic_pg_check_layout(pgio, req);
    856         /* Use full layout for now */
    857         if (!pgio->pg_lseg) {
    858                 ff_layout_pg_get_read(pgio, req, false);
    859                 if (!pgio->pg_lseg)
    860                         goto out_nolseg;
    861         }
    862         if (ff_layout_avoid_read_on_rw(pgio->pg_lseg)) {
    863                 ff_layout_pg_get_read(pgio, req, true);
    864                 if (!pgio->pg_lseg)
    865                         goto out_nolseg;
    866         }
    867         /* Reset wb_nio, since getting layout segment was successful */
    868         req->wb_nio = 0;
    869 
    870         ds = ff_layout_get_ds_for_read(pgio, &ds_idx);
    871         if (!ds) {
                ^^^^^^^^^^
This doesn't check for error pointers either.

    872                 if (!ff_layout_no_fallback_to_mds(pgio->pg_lseg))
    873                         goto out_mds;
    874                 pnfs_generic_pg_cleanup(pgio);
    875                 /* Sleep for 1 second before retrying */
    876                 ssleep(1);
    877                 goto retry;
    878         }
    879 
--> 880         mirror = FF_LAYOUT_COMP(pgio->pg_lseg, ds_idx);
                                                       ^^^^^^
Uninitialized.


regards,
dan carpenter