[PATCH] staging: xillybus: remove redundant Kconfig dependency
XILLYBUS_PCIE and XILLYBUS_OF are inside 'if XILLYBUS' already, so there is not need to depend on XILLYBUS. Cc: Eli Billauer Signed-off-by: Baruch Siach --- drivers/staging/xillybus/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/xillybus/Kconfig b/drivers/staging/xillybus/Kconfig index 8a4181f..b15f778 100644 --- a/drivers/staging/xillybus/Kconfig +++ b/drivers/staging/xillybus/Kconfig @@ -16,14 +16,14 @@ if XILLYBUS config XILLYBUS_PCIE tristate "Xillybus over PCIe" - depends on XILLYBUS && PCI + depends on PCI help Set to M if you want Xillybus to use PCI Express for communicating with the FPGA. config XILLYBUS_OF tristate "Xillybus over Device Tree" - depends on XILLYBUS && OF_ADDRESS && OF_IRQ + depends on OF_ADDRESS && OF_IRQ help Set to M if you want Xillybus to find its resources from the Open Firmware Flattened Device Tree. If the target is an embedded -- 1.8.4.rc3 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/6] v4l: omap4iss: Add support for OMAP4 camera interface - Video devices
On Thu, Oct 03, 2013 at 01:55:29AM +0200, Laurent Pinchart wrote:
> +
> + ret = vb2_streamon(&vfh->queue, type);
> + if (ret < 0)
> + goto err_iss_video_check_format;
> +
> + /* In sensor-to-memory mode, the stream can be started synchronously
> + * to the stream on command. In memory-to-memory mode, it will be
> + * started when buffers are queued on both the input and output.
> + */
> + if (pipe->input == NULL) {
> + unsigned long flags;
> + ret = omap4iss_pipeline_set_stream(pipe,
> + ISS_PIPELINE_STREAM_CONTINUOUS);
> + if (ret < 0)
> + goto err_omap4iss_set_stream;
> + spin_lock_irqsave(&video->qlock, flags);
> + if (list_empty(&video->dmaqueue))
> + video->dmaqueue_flags |= ISS_VIDEO_DMAQUEUE_UNDERRUN;
> + spin_unlock_irqrestore(&video->qlock, flags);
> + }
> +
> + if (ret < 0) {
> +err_omap4iss_set_stream:
> + vb2_streamoff(&vfh->queue, type);
> +err_iss_video_check_format:
> + media_entity_pipeline_stop(&video->video.entity);
> +err_media_entity_pipeline_start:
> + if (video->iss->pdata->set_constraints)
> + video->iss->pdata->set_constraints(video->iss, false);
> + video->queue = NULL;
> + }
> +
> + if (!ret)
> + video->streaming = 1;
> +
> + mutex_unlock(&video->stream_lock);
> + return ret;
> +}
This driver is mostly really nice code, but this error handling is
spaghetti-al-nasty. Just split up the success and failure returns.
video->streaming = 1;
mutex_unlock(&video->stream_lock);
return 0;
err_omap4iss_set_stream:
vb2_streamoff(&vfh->queue, type);
err_iss_video_check_format:
media_entity_pipeline_stop(&video->video.entity);
err_media_entity_pipeline_start:
if (video->iss->pdata->set_constraints)
video->iss->pdata->set_constraints(video->iss, false);
video->queue = NULL;
mutex_unlock(&video->stream_lock);
return ret;
}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Drivers: scsi: FLUSH timeout
On Thu, 2013-10-03 at 13:48 -0700, Eric Seppanen wrote: > On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger > wrote: > > > > On Wed, 2013-10-02 at 18:29 +, KY Srinivasan wrote: > > > Ideally, I want this to be adjustable like the way we can change the I/O > > > timeout. > > > Since that has been attempted earlier and rejected (not clear what the > > > reasons were), > > > I was suggesting that we pick a larger number. James, let me know how I > > > should proceed here. > > > > > > > I think the objection was to making a module parameter for doing this > > globally for all struct scsi_disk, and not the idea of making it > > adjustable on an individual basis per-say.. > > > > What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..? > > Do I/O timeouts and flush timeouts need to be independently adjusted? > If you're having trouble with slow operations, it seems likely to be > across the board. > > Flush timeout could be defined as 2x the read/write timeout. Any > other command-specific timeouts could be scaled the same way. It seems to me that there isn't any reason to expect that the maximum amount of time a device might take to perform various operations are related by any coefficient. And, an HBA (particularly iSCSI or FC) could very well have different device types connected at different target IDs. So I think the flush timeout should be adjustable on a per-device basis. It's probably related more to the cache size on the device than anything else... Also note that there is a SD_WRITE_SAME_TIMEOUT value that is currently 4x the default SD_TIMEOUT value. That should probably be adjustable as well. -Ewan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [dm-devel] dm-writeboost testing
On Fri, 4 Oct 2013, Akira Hayakawa wrote:
> Hi, Mikulas,
>
> I am sorry to say that
> I don't have such machines to reproduce the problem.
>
> But agree with that I am dealing with workqueue subsystem
> in a little bit weird way.
> I should clean them up.
>
> For example,
> free_cache() routine below is
> a deconstructor of the cache metadata
> including all the workqueues.
>
> void free_cache(struct wb_cache *cache)
> {
> cache->on_terminate = true;
>
> /* Kill in-kernel daemons */
> cancel_work_sync(&cache->sync_work);
> cancel_work_sync(&cache->recorder_work);
> cancel_work_sync(&cache->modulator_work);
>
> cancel_work_sync(&cache->flush_work);
> destroy_workqueue(cache->flush_wq);
>
> cancel_work_sync(&cache->barrier_deadline_work);
>
> cancel_work_sync(&cache->migrate_work);
> destroy_workqueue(cache->migrate_wq);
> free_migration_buffer(cache);
>
> /* Destroy in-core structures */
> free_ht(cache);
> free_segment_header_array(cache);
>
> free_rambuf_pool(cache);
> }
>
> cancel_work_sync() before destroy_workqueue()
> can probably be removed because destroy_workqueue() first
> flush all the works.
>
> Although I prepares independent workqueue
> for each flush_work and migrate_work
> other four works are queued into the system_wq
> through schedule_work() routine.
> This asymmetricity is not welcome for
> architecture-portable code.
> Dependencies to the subsystem should be minimized.
> In detail, workqueue subsystem is really changing
> about its concurrency support so
> trusting only the single threaded workqueue
> will be a good idea for stability.
The problem is that you are using workqueues the wrong way. You submit a
work item to a workqueue and the work item is active until the device is
unloaded.
If you submit a work item to a workqueue, it is required that the work
item finishes in finite time. Otherwise, it may stall stall other tasks.
The deadlock when I terminate Xserver is caused by this - the nvidia
driver tries to flush system workqueue and it waits for all work items to
terminate - but your work items don't terminate.
If you need a thread that runs for a long time, you should use
kthread_create, not workqueues (see this
http://people.redhat.com/~mpatocka/patches/kernel/dm-crypt-paralelizace/old-3/dm-crypt-encryption-threads.patch
or this
http://people.redhat.com/~mpatocka/patches/kernel/dm-crypt-paralelizace/old-3/dm-crypt-offload-writes-to-thread.patch
as an example how to use kthreads).
Mikulas
> To begin with,
> these works are never out of queue
> until the deconstructor is called
> but they are repeating running and sleeping.
> Queuing these kind of works to system_wq
> may be unsupported.
>
> So,
> my strategy is to clean them up in a way that
> 1. all daemons are having their own workqueue
> 2. never use cancel_work_sync() but only calls destroy_workqueue()
>in the deconstructor free_cache() and error handling in resume_cache().
>
> Could you please run the same test again
> after I fixed these points
> to see whether it is still reproducible?
>
>
> > On 3.11.3 on PA-RISC without preemption, the device unloads (although it
> > takes many seconds and vmstat shows that the machine is idle during this
> > time)
> This behavior is benign but probably should be improved.
> In said free_cache() it first turns `on_terminate` flag to true
> to notify all the daemons that we are shutting down.
> Since the `update_interval` and `sync_interval` are 60 seconds by default
> we must wait for them to finish for a while.
>
> Akira
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [dm-devel] dm-writeboost testing
Mikulas,
Thanks for your pointing out.
> The problem is that you are using workqueues the wrong way. You submit a
> work item to a workqueue and the work item is active until the device is
> unloaded.
>
> If you submit a work item to a workqueue, it is required that the work
> item finishes in finite time. Otherwise, it may stall stall other tasks.
> The deadlock when I terminate Xserver is caused by this - the nvidia
> driver tries to flush system workqueue and it waits for all work items to
> terminate - but your work items don't terminate.
>
> If you need a thread that runs for a long time, you should use
> kthread_create, not workqueues (see this
> http://people.redhat.com/~mpatocka/patches/kernel/dm-crypt-paralelizace/old-3/dm-crypt-encryption-threads.patch
>
> or this
> http://people.redhat.com/~mpatocka/patches/kernel/dm-crypt-paralelizace/old-3/dm-crypt-offload-writes-to-thread.patch
>
> as an example how to use kthreads).
But I see no reason why you recommend
using a kthread for looping job
instead of putting a looping work item
into a single-threaded not-system workqueue.
For me, they both seem to be working.
Is it documented that
looping job should not be put into
any type of workqueue?
You are only mentioning that
putting a looping work item in system_wq
is the wrong way since
nvidia driver flush the workqueue.
Akira
On 10/4/13 10:38 PM, Mikulas Patocka wrote:
>
>
> On Fri, 4 Oct 2013, Akira Hayakawa wrote:
>
>> Hi, Mikulas,
>>
>> I am sorry to say that
>> I don't have such machines to reproduce the problem.
>>
>> But agree with that I am dealing with workqueue subsystem
>> in a little bit weird way.
>> I should clean them up.
>>
>> For example,
>> free_cache() routine below is
>> a deconstructor of the cache metadata
>> including all the workqueues.
>>
>> void free_cache(struct wb_cache *cache)
>> {
>> cache->on_terminate = true;
>>
>> /* Kill in-kernel daemons */
>> cancel_work_sync(&cache->sync_work);
>> cancel_work_sync(&cache->recorder_work);
>> cancel_work_sync(&cache->modulator_work);
>>
>> cancel_work_sync(&cache->flush_work);
>> destroy_workqueue(cache->flush_wq);
>>
>> cancel_work_sync(&cache->barrier_deadline_work);
>>
>> cancel_work_sync(&cache->migrate_work);
>> destroy_workqueue(cache->migrate_wq);
>> free_migration_buffer(cache);
>>
>> /* Destroy in-core structures */
>> free_ht(cache);
>> free_segment_header_array(cache);
>>
>> free_rambuf_pool(cache);
>> }
>>
>> cancel_work_sync() before destroy_workqueue()
>> can probably be removed because destroy_workqueue() first
>> flush all the works.
>>
>> Although I prepares independent workqueue
>> for each flush_work and migrate_work
>> other four works are queued into the system_wq
>> through schedule_work() routine.
>> This asymmetricity is not welcome for
>> architecture-portable code.
>> Dependencies to the subsystem should be minimized.
>> In detail, workqueue subsystem is really changing
>> about its concurrency support so
>> trusting only the single threaded workqueue
>> will be a good idea for stability.
>
> The problem is that you are using workqueues the wrong way. You submit a
> work item to a workqueue and the work item is active until the device is
> unloaded.
>
> If you submit a work item to a workqueue, it is required that the work
> item finishes in finite time. Otherwise, it may stall stall other tasks.
> The deadlock when I terminate Xserver is caused by this - the nvidia
> driver tries to flush system workqueue and it waits for all work items to
> terminate - but your work items don't terminate.
>
> If you need a thread that runs for a long time, you should use
> kthread_create, not workqueues (see this
> http://people.redhat.com/~mpatocka/patches/kernel/dm-crypt-paralelizace/old-3/dm-crypt-encryption-threads.patch
>
> or this
> http://people.redhat.com/~mpatocka/patches/kernel/dm-crypt-paralelizace/old-3/dm-crypt-offload-writes-to-thread.patch
>
> as an example how to use kthreads).
>
> Mikulas
>
>> To begin with,
>> these works are never out of queue
>> until the deconstructor is called
>> but they are repeating running and sleeping.
>> Queuing these kind of works to system_wq
>> may be unsupported.
>>
>> So,
>> my strategy is to clean them up in a way that
>> 1. all daemons are having their own workqueue
>> 2. never use cancel_work_sync() but only calls destroy_workqueue()
>>in the deconstructor free_cache() and error handling in resume_cache().
>>
>> Could you please run the same test again
>> after I fixed these points
>> to see whether it is still reproducible?
>>
>>
>>> On 3.11.3 on PA-RISC without preemption, the device unloads (although it
>>> takes many seconds and vmstat shows that the machine is idle during this
>>> time)
>> This behavior is benign but probably should be improved.
>> In said free_cache() it first turns `on_terminate` fla
RE: Drivers: scsi: FLUSH timeout
> -Original Message- > From: Eric Seppanen [mailto:e...@purestorage.com] > Sent: Thursday, October 03, 2013 1:49 PM > To: Nicholas A. Bellinger > Cc: KY Srinivasan; linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; > linux-s...@vger.kernel.org > Subject: Re: Drivers: scsi: FLUSH timeout > > On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger > wrote: > > > > On Wed, 2013-10-02 at 18:29 +, KY Srinivasan wrote: > > > Ideally, I want this to be adjustable like the way we can change the I/O > timeout. > > > Since that has been attempted earlier and rejected (not clear what the > reasons were), > > > I was suggesting that we pick a larger number. James, let me know how I > should proceed here. > > > > > > > I think the objection was to making a module parameter for doing this > > globally for all struct scsi_disk, and not the idea of making it > > adjustable on an individual basis per-say.. > > > > What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..? > > Do I/O timeouts and flush timeouts need to be independently adjusted? > If you're having trouble with slow operations, it seems likely to be > across the board. > > Flush timeout could be defined as 2x the read/write timeout. Any > other command-specific timeouts could be scaled the same way. I like this idea and would result in minimal changes. James, if it ok with you, I could send you the patch. Regards, K. Y ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [dm-devel] dm-writeboost testing
On Thu, Oct 03, 2013 at 10:27:54PM +0900, Akira Hayakawa wrote:
> > dm-cache doesn't have this problem, if you overwrite the same piece of
> > data again and again, it goes to the cache device.
>
> It is not a bug but should/can be optimized.
>
> Below is the cache hit path for writes.
> writeboost performs very poorly when a partial write hits
> which then turns `needs_cleanup_perv_cache` to true.
Are you using fixed size blocks for caching then? The whole point of
using a journal/log based disk layout for caching is you can slurp up
all writes irrespective of their size.
What are the scenarios where you out perform dm-cache?
- Joe
> Partial write hits is believed to be unlikely so
> I decided to give up this path to make other likely-paths optimized.
> I think this is just a tradeoff issue of what to be optimized the most.
>
> if (found) {
>
> if (unlikely(on_buffer)) {
> mutex_unlock(&cache->io_lock);
>
> update_mb_idx = mb->idx;
> goto write_on_buffer;
> } else {
> u8 dirty_bits = atomic_read_mb_dirtiness(seg, mb);
>
> /*
> * First clean up the previous cache
> * and migrate the cache if needed.
> */
> bool needs_cleanup_prev_cache =
> !bio_fullsize || !(dirty_bits == 255);
>
> if (unlikely(needs_cleanup_prev_cache)) {
> wait_for_completion(&seg->flush_done);
> migrate_mb(cache, seg, mb, dirty_bits, true);
> }
>
> I checked that the mkfs.ext4 writes only in 4KB size
> so it is not gonna turn the boolean value true for going into the slowpath.
>
> Problem:
> Problem is that
> it chooses the slowpath even though the bio is full-sized overwrite
> in the test.
>
> The reason is that the dirty bits is sometimes seen as 0
> and the suspect is the migration daemon.
>
> I guess you created the writeboost device with the default configuration.
> In that case migration daemon always works and
> some metadata is cleaned up in background.
>
> If you turns both enable_migration_modulator and allow_migrate to 0
> before beginning the test
> to stop migration at all
> it never goes into the slowpath with the test.
>
> Solution:
> Changing the code to
> avoid going into the slowpath when the dirty bits is zero
> will solve this problem.
>
> And done. Please pull the latest one from the repo.
> --- a/Driver/dm-writeboost-target.c
> +++ b/Driver/dm-writeboost-target.c
> @@ -688,6 +688,14 @@ static int writeboost_map(struct dm_target *ti, struct
> bio *bio
> bool needs_cleanup_prev_cache =
> !bio_fullsize || !(dirty_bits == 255);
>
> + /*
> +* Migration works in background
> +* and may have cleaned up the metablock.
> +* If the metablock is clean we need not to migrate.
> +*/
> + if (!dirty_bits)
> + needs_cleanup_prev_cache = false;
> +
> if (unlikely(needs_cleanup_prev_cache)) {
> wait_for_completion(&seg->flush_done);
> migrate_mb(cache, seg, mb, dirty_bits, true);
>
> Thanks,
> Akira
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [dm-devel] dm-writeboost testing
On Fri, 4 Oct 2013, Akira Hayakawa wrote: > Mikulas, > > Thanks for your pointing out. > > > The problem is that you are using workqueues the wrong way. You submit a > > work item to a workqueue and the work item is active until the device is > > unloaded. > > > > If you submit a work item to a workqueue, it is required that the work > > item finishes in finite time. Otherwise, it may stall stall other tasks. > > The deadlock when I terminate Xserver is caused by this - the nvidia > > driver tries to flush system workqueue and it waits for all work items to > > terminate - but your work items don't terminate. > > > > If you need a thread that runs for a long time, you should use > > kthread_create, not workqueues (see this > > http://people.redhat.com/~mpatocka/patches/kernel/dm-crypt-paralelizace/old-3/dm-crypt-encryption-threads.patch > > > > or this > > http://people.redhat.com/~mpatocka/patches/kernel/dm-crypt-paralelizace/old-3/dm-crypt-offload-writes-to-thread.patch > > > > as an example how to use kthreads). > > But I see no reason why you recommend > using a kthread for looping job > instead of putting a looping work item > into a single-threaded not-system workqueue. > > For me, they both seem to be working. As I said, the system locks up when it tries to flush the system workqueue. This happens for example when terminating Xwindow with the nvidia binary driver, but it may happen in other parts of the kernel too. The fact that it works in your setup doesn't mean that it is correct. > Is it documented that > looping job should not be put into > any type of workqueue? It is general assumption when workqueues were created. Maybe it's not documented. > You are only mentioning that > putting a looping work item in system_wq > is the wrong way since > nvidia driver flush the workqueue. > > Akira Mikulas ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: dgnc: changes arguments in sizeof
The arguments that were passed into sizeof were
generic. This patch changes this by putting
the actual item that we need a size of instead.
For example:
- kzalloc(sizeof(struct dgnc_board), GFP_KERNEL);
+ kzalloc(sizeof(*brd), GFP_KERNEL);
Signed-off-by: Lidza Louina
---
drivers/staging/dgnc/dgnc_driver.c | 4 ++--
drivers/staging/dgnc/dgnc_mgmt.c | 2 +-
drivers/staging/dgnc/dgnc_tty.c| 24
3 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/dgnc/dgnc_driver.c
b/drivers/staging/dgnc/dgnc_driver.c
index c398193..4271fa3 100644
--- a/drivers/staging/dgnc/dgnc_driver.c
+++ b/drivers/staging/dgnc/dgnc_driver.c
@@ -487,7 +487,7 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
/* get the board structure and prep it */
brd = dgnc_Board[dgnc_NumBoards] =
- kzalloc(sizeof(struct dgnc_board), GFP_KERNEL);
+ kzalloc(sizeof(*brd), GFP_KERNEL);
if (!brd) {
APR(("memory allocation for board structure failed\n"));
return -ENOMEM;
@@ -495,7 +495,7 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
/* make a temporary message buffer for the boot messages */
brd->msgbuf = brd->msgbuf_head =
- kzalloc(sizeof(char) * 8192, GFP_KERNEL);
+ kzalloc(sizeof(u8) * 8192, GFP_KERNEL);
if (!brd->msgbuf) {
kfree(brd);
APR(("memory allocation for board msgbuf failed\n"));
diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c
index bb39f5d..354458c 100644
--- a/drivers/staging/dgnc/dgnc_mgmt.c
+++ b/drivers/staging/dgnc/dgnc_mgmt.c
@@ -209,7 +209,7 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
uint board = 0;
uint channel = 0;
- if (copy_from_user(&ni, uarg, sizeof(struct ni_info))) {
+ if (copy_from_user(&ni, uarg, sizeof(ni))) {
return -EFAULT;
}
diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 1350d62..c6fee11 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -200,8 +200,8 @@ int dgnc_tty_register(struct dgnc_board *brd)
DPR_INIT(("tty_register start\n"));
- memset(&brd->SerialDriver, 0, sizeof(struct tty_driver));
- memset(&brd->PrintDriver, 0, sizeof(struct tty_driver));
+ memset(&brd->SerialDriver, 0, sizeof(brd->SerialDriver));
+ memset(&brd->PrintDriver, 0, sizeof(brd->PrintDriver));
brd->SerialDriver.magic = TTY_DRIVER_MAGIC;
@@ -222,12 +222,12 @@ int dgnc_tty_register(struct dgnc_board *brd)
* The kernel wants space to store pointers to
* tty_struct's and termios's.
*/
- brd->SerialDriver.ttys = kzalloc(brd->maxports * sizeof(struct
tty_struct *), GFP_KERNEL);
+ brd->SerialDriver.ttys = kzalloc(brd->maxports *
sizeof(*brd->SerialDriver.ttys), GFP_KERNEL);
if (!brd->SerialDriver.ttys)
return -ENOMEM;
kref_init(&brd->SerialDriver.kref);
- brd->SerialDriver.termios = kzalloc(brd->maxports * sizeof(struct
ktermios *), GFP_KERNEL);
+ brd->SerialDriver.termios = kzalloc(brd->maxports *
sizeof(*brd->SerialDriver.termios), GFP_KERNEL);
if (!brd->SerialDriver.termios)
return -ENOMEM;
@@ -271,11 +271,11 @@ int dgnc_tty_register(struct dgnc_board *brd)
* tty_struct's and termios's. Must be separated from
* the Serial Driver so we don't get confused
*/
- brd->PrintDriver.ttys = kzalloc(brd->maxports * sizeof(struct
tty_struct *), GFP_KERNEL);
+ brd->PrintDriver.ttys = kzalloc(brd->maxports *
sizeof(*brd->PrintDriver.ttys), GFP_KERNEL);
if (!brd->PrintDriver.ttys)
return -ENOMEM;
kref_init(&brd->PrintDriver.kref);
- brd->PrintDriver.termios = kzalloc(brd->maxports * sizeof(struct
ktermios *), GFP_KERNEL);
+ brd->PrintDriver.termios = kzalloc(brd->maxports *
sizeof(*brd->PrintDriver.termios), GFP_KERNEL);
if (!brd->PrintDriver.termios)
return -ENOMEM;
@@ -341,7 +341,7 @@ int dgnc_tty_init(struct dgnc_board *brd)
* Okay to malloc with GFP_KERNEL, we are not at
* interrupt context, and there are no locks held.
*/
- brd->channels[i] = kzalloc(sizeof(struct channel_t),
GFP_KERNEL);
+ brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
GFP_KERNEL);
if (!brd->channels[i]) {
DPR_CORE(("%s:%d Unable to allocate memory for
channel struct\n",
__FILE__, __LINE__));
@@ -2664,7 +2664,7 @@ static int dgnc_tty_digiseta(struct tty_struct *tty,
struct digi_t __user *new_i
Re: Drivers: scsi: FLUSH timeout
On Fri, 2013-10-04 at 15:02 +, KY Srinivasan wrote: > > > -Original Message- > > From: Eric Seppanen [mailto:e...@purestorage.com] > > Sent: Thursday, October 03, 2013 1:49 PM > > To: Nicholas A. Bellinger > > Cc: KY Srinivasan; linux-ker...@vger.kernel.org; > > de...@linuxdriverproject.org; > > linux-s...@vger.kernel.org > > Subject: Re: Drivers: scsi: FLUSH timeout > > > > On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger > > wrote: > > > > > > On Wed, 2013-10-02 at 18:29 +, KY Srinivasan wrote: > > > > Ideally, I want this to be adjustable like the way we can change the I/O > > timeout. > > > > Since that has been attempted earlier and rejected (not clear what the > > reasons were), > > > > I was suggesting that we pick a larger number. James, let me know how I > > should proceed here. > > > > > > > > > > I think the objection was to making a module parameter for doing this > > > globally for all struct scsi_disk, and not the idea of making it > > > adjustable on an individual basis per-say.. > > > > > > What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..? > > > > Do I/O timeouts and flush timeouts need to be independently adjusted? > > If you're having trouble with slow operations, it seems likely to be > > across the board. > > > > Flush timeout could be defined as 2x the read/write timeout. Any > > other command-specific timeouts could be scaled the same way. > > I like this idea and would result in minimal changes. James, if it ok with > you, > I could send you the patch. Depends: I still prefer the per-target override, but if the proposal is to take the existing variable timeout for the queue and have 2x that for the flush, so you plan to increase the per-device timeout with hyper-v to 90s via sysfs, then I'm OK with it. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Drivers: scsi: FLUSH timeout
On Fri, Oct 4, 2013 at 5:18 AM, Ewan Milne wrote: > On Thu, 2013-10-03 at 13:48 -0700, Eric Seppanen wrote: >> Do I/O timeouts and flush timeouts need to be independently adjusted? >> If you're having trouble with slow operations, it seems likely to be >> across the board. >> >> Flush timeout could be defined as 2x the read/write timeout. Any >> other command-specific timeouts could be scaled the same way. > > It seems to me that there isn't any reason to expect that the maximum > amount of time a device might take to perform various operations are > related by any coefficient. And, an HBA (particularly iSCSI or FC) > could very well have different device types connected at different > target IDs. So I think the flush timeout should be adjustable on > a per-device basis. It's probably related more to the cache size > on the device than anything else... There are two possible delays: how long the device might possibly take, and how long the storage fabric might take. On a local device, only the first matters. But there are environments where the second dominates (e.g. a virtual machine, where the hypervisor's storage uses multipath with a long failover delay). If somebody wants to set flush timeouts > 60 seconds, I would like to know if they're trying to address a slow device or a slow fabric. If it's the fabric, then it's kind of silly to make them set three different timeouts to address the same problem. An alternate way of handling long fabric delays would be to have a fabric_timeout that gets added to all the other timeouts... could be a scsi_host parameter but that's probably overengineering the problem. There are already VM vendors that tell customers to adjust the current sysfs timeout, so the least amount of work would be to make all of the other timeouts track that one in some way (additive or multiplicative). ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic I/O timeout
Rather than having a separate constant for specifying the timeout on FLUSH
operations, use the basic I/O timeout value that is already configurable
on a per target basis to derive the FLUSH timeout. Looking at the current
definitions of these timeout values, the FLUSH operation is supposed to have
a value that is twice the normal timeout value. This patch preserves this
relationship while leveraging the flexibility of specifying the I/O timeout.
I would like to thank Eric Seppanen and
Nicholas A. Bellinger for their help in resolving
this issue.
Signed-off-by: K. Y. Srinivasan
---
drivers/scsi/sd.c |5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e62d17d..8aff306 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
*sdp, struct request *rq)
static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
{
- rq->timeout = SD_FLUSH_TIMEOUT;
+ rq->timeout *= 2;
rq->retries = SD_MAX_RETRIES;
rq->cmd[0] = SYNCHRONIZE_CACHE;
rq->cmd_len = 10;
@@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
{
int retries, res;
struct scsi_device *sdp = sdkp->device;
+ unsigned int timeout = sdp->request_queue->rq_timeout;
struct scsi_sense_hdr sshdr;
if (!scsi_device_online(sdp))
@@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
* flush everything.
*/
res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
-&sshdr, SD_FLUSH_TIMEOUT,
+&sshdr, timeout * 2,
SD_MAX_RETRIES, NULL, REQ_PM);
if (res == 0)
break;
--
1.7.4.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2013-10-04 at 12:38 -0700, K. Y. Srinivasan wrote:
> Rather than having a separate constant for specifying the timeout on FLUSH
> operations, use the basic I/O timeout value that is already configurable
> on a per target basis to derive the FLUSH timeout. Looking at the current
> definitions of these timeout values, the FLUSH operation is supposed to have
> a value that is twice the normal timeout value. This patch preserves this
> relationship while leveraging the flexibility of specifying the I/O timeout.
>
> I would like to thank Eric Seppanen and
> Nicholas A. Bellinger for their help in resolving
> this issue.
>
> Signed-off-by: K. Y. Srinivasan
> ---
> drivers/scsi/sd.c |5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..8aff306 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
> *sdp, struct request *rq)
>
> static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
> {
> - rq->timeout = SD_FLUSH_TIMEOUT;
> + rq->timeout *= 2;
> rq->retries = SD_MAX_RETRIES;
> rq->cmd[0] = SYNCHRONIZE_CACHE;
> rq->cmd_len = 10;
> @@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> {
> int retries, res;
> struct scsi_device *sdp = sdkp->device;
> + unsigned int timeout = sdp->request_queue->rq_timeout;
The timeout is signed in the function prototype
> struct scsi_sense_hdr sshdr;
>
> if (!scsi_device_online(sdp))
> @@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>* flush everything.
>*/
> res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> - &sshdr, SD_FLUSH_TIMEOUT,
> + &sshdr, timeout * 2,
>SD_MAX_RETRIES, NULL, REQ_PM);
> if (res == 0)
> break;
Not like this, please: you now leave us with a dangling #define whose
name makes you think it should be related to flushing and a couple of
curious magic constants. It's almost hand crafted to confuse people
reading the code.
Please do it like this instead: with a comment explaining what we're
doing above the #define and a name that clearly relates to the actions.
James
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e62d17d..5c9496d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
*sdp, struct request *rq)
static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
{
- rq->timeout = SD_FLUSH_TIMEOUT;
+ rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
rq->retries = SD_MAX_RETRIES;
rq->cmd[0] = SYNCHRONIZE_CACHE;
rq->cmd_len = 10;
@@ -1433,6 +1433,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
{
int retries, res;
struct scsi_device *sdp = sdkp->device;
+ const int timeout = sdp->request_queue->rq_timeout
+ * SD_FLUSH_TIMEOUT_MULTIPLIER;
struct scsi_sense_hdr sshdr;
if (!scsi_device_online(sdp))
@@ -1448,8 +1450,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
* flush everything.
*/
res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
-&sshdr, SD_FLUSH_TIMEOUT,
-SD_MAX_RETRIES, NULL, REQ_PM);
+&sshdr, timeout, SD_MAX_RETRIES,
+NULL, REQ_PM);
if (res == 0)
break;
}
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 7a049de..7f7999c 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -13,7 +13,11 @@
*/
#define SD_TIMEOUT (30 * HZ)
#define SD_MOD_TIMEOUT (75 * HZ)
-#define SD_FLUSH_TIMEOUT (60 * HZ)
+/*
+ * Flush timeout is a multiplier over the standard device timeout which is
+ * user modifiable via sysfs but initially set to SD_TIMEOUT
+ */
+#define SD_FLUSH_TIMEOUT_MULTIPLIER2
#define SD_WRITE_SAME_TIMEOUT (120 * HZ)
/*
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic I/O timeout
> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Friday, October 04, 2013 2:42 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org;
> e...@purestorage.com; n...@linux-iscsi.org; linux-s...@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the
> basic
> I/O timeout
>
> On Fri, 2013-10-04 at 12:38 -0700, K. Y. Srinivasan wrote:
> > Rather than having a separate constant for specifying the timeout on FLUSH
> > operations, use the basic I/O timeout value that is already configurable
> > on a per target basis to derive the FLUSH timeout. Looking at the current
> > definitions of these timeout values, the FLUSH operation is supposed to have
> > a value that is twice the normal timeout value. This patch preserves this
> > relationship while leveraging the flexibility of specifying the I/O timeout.
> >
> > I would like to thank Eric Seppanen and
> > Nicholas A. Bellinger for their help in resolving
> > this issue.
> >
> > Signed-off-by: K. Y. Srinivasan
> > ---
> > drivers/scsi/sd.c |5 +++--
> > 1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index e62d17d..8aff306 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct
> scsi_device *sdp, struct request *rq)
> >
> > static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request
> > *rq)
> > {
> > - rq->timeout = SD_FLUSH_TIMEOUT;
> > + rq->timeout *= 2;
> > rq->retries = SD_MAX_RETRIES;
> > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > rq->cmd_len = 10;
> > @@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > {
> > int retries, res;
> > struct scsi_device *sdp = sdkp->device;
> > + unsigned int timeout = sdp->request_queue->rq_timeout;
>
> The timeout is signed in the function prototype
>
> > struct scsi_sense_hdr sshdr;
> >
> > if (!scsi_device_online(sdp))
> > @@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > * flush everything.
> > */
> > res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> > -&sshdr, SD_FLUSH_TIMEOUT,
> > +&sshdr, timeout * 2,
> > SD_MAX_RETRIES, NULL, REQ_PM);
> > if (res == 0)
> > break;
>
> Not like this, please: you now leave us with a dangling #define whose
> name makes you think it should be related to flushing and a couple of
> curious magic constants. It's almost hand crafted to confuse people
> reading the code.
>
> Please do it like this instead: with a comment explaining what we're
> doing above the #define and a name that clearly relates to the actions.
>
> James
>
> ---
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..5c9496d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
> *sdp, struct request *rq)
>
> static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
> {
> - rq->timeout = SD_FLUSH_TIMEOUT;
> + rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> rq->retries = SD_MAX_RETRIES;
> rq->cmd[0] = SYNCHRONIZE_CACHE;
> rq->cmd_len = 10;
> @@ -1433,6 +1433,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> {
> int retries, res;
> struct scsi_device *sdp = sdkp->device;
> + const int timeout = sdp->request_queue->rq_timeout
> + * SD_FLUSH_TIMEOUT_MULTIPLIER;
> struct scsi_sense_hdr sshdr;
>
> if (!scsi_device_online(sdp))
> @@ -1448,8 +1450,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>* flush everything.
>*/
> res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> - &sshdr, SD_FLUSH_TIMEOUT,
> - SD_MAX_RETRIES, NULL, REQ_PM);
> + &sshdr, timeout, SD_MAX_RETRIES,
> + NULL, REQ_PM);
> if (res == 0)
> break;
> }
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 7a049de..7f7999c 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -13,7 +13,11 @@
> */
> #define SD_TIMEOUT (30 * HZ)
> #define SD_MOD_TIMEOUT (75 * HZ)
> -#define SD_FLUSH_TIMEOUT (60 * HZ)
> +/*
> + * Flush timeout is a multiplier over the standard device timeout which is
> + * user modifiable via sysfs but initially set to SD_TIMEOUT
> + */
> +#define SD_FLUSH_TIMEOUT_MULTIPLIER 2
> #define SD_WRITE_SAME_TIMEOUT(120 * HZ)
>
> /*
Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2013-10-04 at 22:01 +, KY Srinivasan wrote:
>
> > -Original Message-
> > From: James Bottomley [mailto:jbottom...@parallels.com]
> > Sent: Friday, October 04, 2013 2:42 PM
> > To: KY Srinivasan
> > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org;
> > e...@purestorage.com; n...@linux-iscsi.org; linux-s...@vger.kernel.org
> > Subject: Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the
> > basic
> > I/O timeout
> >
> > On Fri, 2013-10-04 at 12:38 -0700, K. Y. Srinivasan wrote:
> > > Rather than having a separate constant for specifying the timeout on FLUSH
> > > operations, use the basic I/O timeout value that is already configurable
> > > on a per target basis to derive the FLUSH timeout. Looking at the current
> > > definitions of these timeout values, the FLUSH operation is supposed to
> > > have
> > > a value that is twice the normal timeout value. This patch preserves this
> > > relationship while leveraging the flexibility of specifying the I/O
> > > timeout.
> > >
> > > I would like to thank Eric Seppanen and
> > > Nicholas A. Bellinger for their help in resolving
> > > this issue.
> > >
> > > Signed-off-by: K. Y. Srinivasan
> > > ---
> > > drivers/scsi/sd.c |5 +++--
> > > 1 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > > index e62d17d..8aff306 100644
> > > --- a/drivers/scsi/sd.c
> > > +++ b/drivers/scsi/sd.c
> > > @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct
> > scsi_device *sdp, struct request *rq)
> > >
> > > static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request
> > > *rq)
> > > {
> > > - rq->timeout = SD_FLUSH_TIMEOUT;
> > > + rq->timeout *= 2;
> > > rq->retries = SD_MAX_RETRIES;
> > > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > > rq->cmd_len = 10;
> > > @@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > > {
> > > int retries, res;
> > > struct scsi_device *sdp = sdkp->device;
> > > + unsigned int timeout = sdp->request_queue->rq_timeout;
> >
> > The timeout is signed in the function prototype
> >
> > > struct scsi_sense_hdr sshdr;
> > >
> > > if (!scsi_device_online(sdp))
> > > @@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > >* flush everything.
> > >*/
> > > res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> > > - &sshdr, SD_FLUSH_TIMEOUT,
> > > + &sshdr, timeout * 2,
> > >SD_MAX_RETRIES, NULL, REQ_PM);
> > > if (res == 0)
> > > break;
> >
> > Not like this, please: you now leave us with a dangling #define whose
> > name makes you think it should be related to flushing and a couple of
> > curious magic constants. It's almost hand crafted to confuse people
> > reading the code.
> >
> > Please do it like this instead: with a comment explaining what we're
> > doing above the #define and a name that clearly relates to the actions.
> >
> > James
> >
> > ---
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index e62d17d..5c9496d 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
> > *sdp, struct request *rq)
> >
> > static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request
> > *rq)
> > {
> > - rq->timeout = SD_FLUSH_TIMEOUT;
> > + rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> > rq->retries = SD_MAX_RETRIES;
> > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > rq->cmd_len = 10;
> > @@ -1433,6 +1433,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > {
> > int retries, res;
> > struct scsi_device *sdp = sdkp->device;
> > + const int timeout = sdp->request_queue->rq_timeout
> > + * SD_FLUSH_TIMEOUT_MULTIPLIER;
> > struct scsi_sense_hdr sshdr;
> >
> > if (!scsi_device_online(sdp))
> > @@ -1448,8 +1450,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > * flush everything.
> > */
> > res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> > -&sshdr, SD_FLUSH_TIMEOUT,
> > -SD_MAX_RETRIES, NULL, REQ_PM);
> > +&sshdr, timeout, SD_MAX_RETRIES,
> > +NULL, REQ_PM);
> > if (res == 0)
> > break;
> > }
> > diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> > index 7a049de..7f7999c 100644
> > --- a/drivers/scsi/sd.h
> > +++ b/drivers/scsi/sd.h
> > @@ -13,7 +13,11 @@
> > */
> > #define SD_TIMEOUT (30 * HZ)
> > #define SD_MOD_TIMEOUT (75 * HZ)
> > -#define SD_FLUSH_TIMEOUT (60 * HZ)
> > +/*
> > + * Flush time
RE: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic I/O timeout
> -Original Message-
> From: James Bottomley [mailto:jbottom...@parallels.com]
> Sent: Friday, October 04, 2013 3:31 PM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org;
> e...@purestorage.com; n...@linux-iscsi.org; linux-s...@vger.kernel.org
> Subject: Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the
> basic
> I/O timeout
>
> On Fri, 2013-10-04 at 22:01 +, KY Srinivasan wrote:
> >
> > > -Original Message-
> > > From: James Bottomley [mailto:jbottom...@parallels.com]
> > > Sent: Friday, October 04, 2013 2:42 PM
> > > To: KY Srinivasan
> > > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > > de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org;
> > > e...@purestorage.com; n...@linux-iscsi.org; linux-s...@vger.kernel.org
> > > Subject: Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the
> basic
> > > I/O timeout
> > >
> > > On Fri, 2013-10-04 at 12:38 -0700, K. Y. Srinivasan wrote:
> > > > Rather than having a separate constant for specifying the timeout on
> > > > FLUSH
> > > > operations, use the basic I/O timeout value that is already configurable
> > > > on a per target basis to derive the FLUSH timeout. Looking at the
> > > > current
> > > > definitions of these timeout values, the FLUSH operation is supposed to
> have
> > > > a value that is twice the normal timeout value. This patch preserves
> > > > this
> > > > relationship while leveraging the flexibility of specifying the I/O
> > > > timeout.
> > > >
> > > > I would like to thank Eric Seppanen and
> > > > Nicholas A. Bellinger for their help in resolving
> > > > this issue.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan
> > > > ---
> > > > drivers/scsi/sd.c |5 +++--
> > > > 1 files changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > > > index e62d17d..8aff306 100644
> > > > --- a/drivers/scsi/sd.c
> > > > +++ b/drivers/scsi/sd.c
> > > > @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct
> > > scsi_device *sdp, struct request *rq)
> > > >
> > > > static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct
> > > > request
> *rq)
> > > > {
> > > > - rq->timeout = SD_FLUSH_TIMEOUT;
> > > > + rq->timeout *= 2;
> > > > rq->retries = SD_MAX_RETRIES;
> > > > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > > > rq->cmd_len = 10;
> > > > @@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > > > {
> > > > int retries, res;
> > > > struct scsi_device *sdp = sdkp->device;
> > > > + unsigned int timeout = sdp->request_queue->rq_timeout;
> > >
> > > The timeout is signed in the function prototype
> > >
> > > > struct scsi_sense_hdr sshdr;
> > > >
> > > > if (!scsi_device_online(sdp))
> > > > @@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > > > * flush everything.
> > > > */
> > > > res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL,
> > > > 0,
> > > > -&sshdr, SD_FLUSH_TIMEOUT,
> > > > +&sshdr, timeout * 2,
> > > > SD_MAX_RETRIES, NULL,
> > > > REQ_PM);
> > > > if (res == 0)
> > > > break;
> > >
> > > Not like this, please: you now leave us with a dangling #define whose
> > > name makes you think it should be related to flushing and a couple of
> > > curious magic constants. It's almost hand crafted to confuse people
> > > reading the code.
> > >
> > > Please do it like this instead: with a comment explaining what we're
> > > doing above the #define and a name that clearly relates to the actions.
> > >
> > > James
> > >
> > > ---
> > >
> > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > > index e62d17d..5c9496d 100644
> > > --- a/drivers/scsi/sd.c
> > > +++ b/drivers/scsi/sd.c
> > > @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct
> scsi_device
> > > *sdp, struct request *rq)
> > >
> > > static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request
> > > *rq)
> > > {
> > > - rq->timeout = SD_FLUSH_TIMEOUT;
> > > + rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> > > rq->retries = SD_MAX_RETRIES;
> > > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > > rq->cmd_len = 10;
> > > @@ -1433,6 +1433,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > > {
> > > int retries, res;
> > > struct scsi_device *sdp = sdkp->device;
> > > + const int timeout = sdp->request_queue->rq_timeout
> > > + * SD_FLUSH_TIMEOUT_MULTIPLIER;
> > > struct scsi_sense_hdr sshdr;
> > >
> > > if (!scsi_device_online(sdp))
> > > @@ -1448,8 +1450,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > >* flush everything.
> > >
Re: lustre: why does cfs_get_random_bytes() exist?
On 2013/10/03 5:45 PM, "Theodore Ts'o" wrote: >On Thu, Oct 03, 2013 at 11:06:58PM +, Dilger, Andreas wrote: >> >> The Lustre cfs_get_random_bytes() incorporates (via cfs_rand()) a seed >> which also hashes in the addresses from any network interfaces that are >> configured. >> Conversely, cfs_rand() also is seeded at startup from >>get_random_bytes() in >> case a hardware RNG is available. This ensures even with identical >>initial >> conditions cfs_get_random_bytes() gets a different random stream on each >> node. > >With modern kernels, the /dev/random driver has the >add_device_randomness() interface which is used to mix in >personalization information, which includes the network MAC address. >So that particular concern should be covered without the hack of >mixing in cfs_rand(). I think that depends on the network driver. The Cray systems have some very strange networking hardware that is beyond our control - definitely not ethernet or Infiniband. I'll have to ask the Cray folks if their network drivers do this today. >> I'm not against cleaning this up, if there is some mechanism for the >> startup code to add in the node interface addresses into the entropy >> pool, and this is also used to perturb the prandom_u32() sequence >> after that point. > >That's handled too, via the late initcall prandom_reseed(). > >Cheers, > > - Ted > Cheers, Andreas -- Andreas Dilger Lustre Software Architect Intel High Performance Data Division ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
