On Sun, Oct 16, 2016 at 10:19:53AM +0200, Stefan Fritsch wrote:
> On Sun, 16 Oct 2016, Stefan Fritsch wrote:
> > * When a R/W mount is updated to R/O. I will send patches for this in a 
> > separate mail.
> 
> Part 1: Add an ioctl:
> 
> 
> add DIOCCACHESYNC
>     
> Add an ioctl to tell storage devices to flush their internal caches.
> Ported from netbsd by pedro@
> 
> OK?

Some remarks inline

> @@ -848,6 +848,10 @@ wdioctl(dev_t dev, u_long xfer, caddr_t addr, int flag, 
> struct proc *p)
>               }
>  #endif
>  
> +     /* XXX pedro: should set AT_WAIT according to force flag */

Does this comment help?  Either do (*(int *)addr ?  AT_WAIT : 0)
or just pass AT_WAIT.  But don't force all people reading the code
to think about it.

I think the AT_WAIT code is correct and the comment is wrong.  In
sdioctl() the force flag overrides the SDF_DIRTY flag.  This is
different from setting AT_WAIT.  Just remove the pedro comment.
Then we have the NetBSD code.

> +     case DIOCCACHESYNC:
> +             return wd_flushcache(wd, AT_WAIT);
> +

All other cases have a "goto exit".  With the return we miss a
device_unref().

>       default:
>               error = wdc_ioctl(wd->drvp, xfer, addr, flag, p);
>               goto exit;


> @@ -1067,13 +1071,13 @@ wd_get_params(struct wd_softc *wd, u_int8_t flags, 
> struct ataparams *params)
>       }
>  }
>  
> -void
> +int
>  wd_flushcache(struct wd_softc *wd, int flags)
>  {
>       struct wdc_command wdc_c;
>  
>       if (wd->drvp->ata_vers < 4) /* WDCC_FLUSHCACHE is here since ATA-4 */
> -             return;
> +             return EIO;
>       bzero(&wdc_c, sizeof(struct wdc_command));
>       wdc_c.r_command = (wd->sc_flags & WDF_LBA48 ? WDCC_FLUSHCACHE_EXT :
>           WDCC_FLUSHCACHE);
> @@ -1088,20 +1092,18 @@ wd_flushcache(struct wd_softc *wd, int flags)
>       if (wdc_exec_command(wd->drvp, &wdc_c) != WDC_COMPLETE) {
>               printf("%s: flush cache command didn't complete\n",
>                   wd->sc_dev.dv_xname);
> +             return EIO;
>       }
> -     if (wdc_c.flags & AT_TIMEOU) {
> -             printf("%s: flush cache command timeout\n",
> -                 wd->sc_dev.dv_xname);
> +     if (wdc_c.flags & AT_ERROR) {
> +             if (wdc_c.r_error == WDCE_ABRT) /* command not supported */
> +                     return ENODEV;
>       }
> -     if (wdc_c.flags & AT_DF) {
> -             printf("%s: flush cache command: drive fault\n",
> +     if (wdc_c.flags & (AT_ERROR | AT_TIMEOU | AT_DF)) {
> +             printf("%s: flush cache command timeout\n",
>                   wd->sc_dev.dv_xname);

NetBSD prints the flags here.  If we merge the cases, debugging
will be harder if don't print the relevant bits.

> +             return EIO;
>       }
> -     /*
> -      * Ignore error register, it shouldn't report anything else
> -      * than COMMAND ABORTED, which means the device doesn't support
> -      * flush cache
> -      */
> +     return 0;
>  }

Instead of merging the cases, which was done independently in NetBSD,
I would perfer to take their original commit.

http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ata/wd.c.diff?r1=1.275&r2=1.276&only_with_tag=MAIN&f=h

Also I think their EIO/ENODEV choices are better.

> @@ -999,6 +999,15 @@ sdioctl(dev_t dev, u_long cmd, caddr_t addr, int flag, 
> struct proc *p)
>               error = sd_ioctl_cache(sc, cmd, (struct dk_cache *)addr);
>               goto exit;
>  
> +     case DIOCCACHESYNC:
> +             if (!ISSET(flag, FWRITE)) {
> +                     error = EBADF;
> +                     goto exit;
> +             }
> +             if ((sc->flags & SDF_DIRTY) != 0 || *(int *)addr != 0)
> +                     error = sd_flush(sc, 0);
> +             return (error);

You need a goto exit to call device_unref().

> +
>       default:
>               if (part != RAW_PART) {
>                       error = ENOTTY;

bluhm

Reply via email to