Re: [PATCH linux-2.6-block:post-2.6.15 09/10] blk: add FUA support to IDE

[Tejun Heo <htejun@gmail.com>]

Hi, Bartlomiej.

Bartlomiej Zolnierkiewicz wrote:
> On 11/17/05, Tejun Heo <htejun@gmail.com> wrote:
> 
> What does happen for fua && drive->vdma case?
> 

Thanks for pointing out, wasn't thinking about that.  Hmmm... When using 
vdma, single-sector PIO commands are issued instead but there's no 
single-sector FUA PIO command.  Would issuing 
ATA_CMD_WRITE_MULTI_FUA_EXT instead of ATA_CMD_WRITE_FUA_EXT work?  Or 
should I just disable FUA on vdma case?

> 
>>                        } else {
>>                                command = lba48 ? WIN_READDMA_EXT : WIN_READDMA;
>>                                if (drive->vdma)
>>@@ -284,8 +298,20 @@ static ide_startstop_t __ide_do_rw_disk(
>>        } else {
>>                if (drive->mult_count) {
>>                        hwif->data_phase = TASKFILE_MULTI_OUT;
>>-                       command = lba48 ? WIN_MULTWRITE_EXT : WIN_MULTWRITE;
>>+                       if (!fua)
>>+                               command = lba48 ? WIN_MULTWRITE_EXT : WIN_MULTWRITE;
>>+                       else
>>+                               command = ATA_CMD_WRITE_MULTI_FUA_EXT;
>>                } else {
>>+                       if (unlikely(fua)) {
>>+                               /*
>>+                                * This happens if multisector PIO is
>>+                                * turned off during operation.
>>+                                */
>>+                               printk(KERN_ERR "%s: FUA write but in single "
>>+                                      "sector PIO mode\n", drive->name);
>>+                               goto fail;
>>+                       }
> 
> 
> Wouldn't it be better to do the following check at the beginning
> of __ide_do_rw_disk() (after checking for dma vs lba48):
> 
>         if (fua) {
>                 if (!lba48 || ((!dma || drive->vdma) && !drive->mult_count))
>                         goto fail_fua;
>         }
> 
> ...
> 
> and fail the request if needed *before* actually touching any
> hardware registers?
> 
> fail_fua:
>         printk(KERN_ERR "%s: FUA write unsupported (lba48=%u dma=%u"
>                                        " vdma=%u mult_count=%u)\n", drive->name,
>                                        lba48, dma, drive->vdma,
> drive->mult_count);
>         ide_end_request(drive, 0, 0);
>         return ide_stopped;
> 

Hmmm... The thing is that those failure cases will happen extremely 
rarely if at all.  Remember this post?

http://marc.theaimsgroup.com/?l=linux-kernel&m=111798102108338&w=3

It's mostly guaranteed that those failure cases don't occur, so I 
thought avoiding IO on failure case wasn't that helpful.

> 
>>                        hwif->data_phase = TASKFILE_OUT;
>>                        command = lba48 ? WIN_WRITE_EXT : WIN_WRITE;
>>                }
>>@@ -295,6 +321,10 @@ static ide_startstop_t __ide_do_rw_disk(
>>
>>                return pre_task_out_intr(drive, rq);
>>        }
>>+
>>+ fail:
>>+       ide_end_request(drive, 0, 0);
>>+       return ide_stopped;
>> }
>>
>> /*
>>@@ -846,7 +876,7 @@ static void idedisk_setup (ide_drive_t *
>> {
>>        struct hd_driveid *id = drive->id;
>>        unsigned long long capacity;
>>-       int barrier;
>>+       int barrier, fua;
>>
>>        idedisk_add_settings(drive);
>>
>>@@ -967,10 +997,19 @@ static void idedisk_setup (ide_drive_t *
>>                        barrier = 0;
>>        }
>>
>>-       printk(KERN_INFO "%s: cache flushes %ssupported\n",
>>-               drive->name, barrier ? "" : "not ");
>>+       fua = barrier && idedisk_supports_lba48(id) && ide_id_has_fua(id);
>>+       /* When using PIO, FUA needs multisector. */
>>+       if ((!drive->using_dma || drive->hwif->no_lba48_dma) &&
>>+           drive->mult_count == 0)
>>+               fua = 0;
> 
> 
> Shouldn't this check also for drive->vdma?
> 

Yes, it does.  Thanks for pointing out.  One question though.  FUA 
support should be changed if using_dma/mult_count settings are changed. 
  As using_dma configuration is handled by IDE midlayer, we might need 
to add a callback there.  What do you think?

-- 
tejun