Hi Bart,
here's the 1st draft of the pipeline removal series. As the diffstat below openly
states it, a lot of code got removed - even more than the cleanup series we did
earlier. There are several issues that we need to address concerning these
patches:
1. only compile-tested since i don't have the hardware, i.e. longer -mm brewing is
advisable the least.
2. I have left the tape->merge_stage buffer structure along with its
alloc/free functions intact for now, for simplicity. The next step would be
to go and carefully audit the code and then remove that last piece
too and use allocations on the stack instead. I guess we still expect
Jens's response on whether blk_{get,put}_request is the way to go here.
Jens?
3. After applying those we're moving into the direction of a lightweight
ide-tape driver so the stanza for its removal in
Documentation/feature-removal-schedule.txt becomes incorrect.
4. ... (i'm sure you're gonna have some more :))
Documentation/ide/ide-tape.txt | 211 +++------
drivers/ide/ide-tape.c | 1032 +++-------------------------------------
2 files changed, 134 insertions(+), 1109 deletions(-)
@@ -206,19 +206,15 @@ enum {/* 0 When the tape position is unknown */IDETAPE_FLAG_ADDRESS_VALID=(1<<1),/* Device already opened */-IDETAPE_FLAG_BUSY=(1<<2),-/* Error detected in a pipeline stage */-IDETAPE_FLAG_PIPELINE_ERR=(1<<3),+IDETAPE_FLAG_BUSY=(1<<2),/* Attempt to auto-detect the current user block size */-IDETAPE_FLAG_DETECT_BS=(1<<4),+IDETAPE_FLAG_DETECT_BS=(1<<3),/* Currently on a filemark */-IDETAPE_FLAG_FILEMARK=(1<<5),+IDETAPE_FLAG_FILEMARK=(1<<4),/* DRQ interrupt device */-IDETAPE_FLAG_DRQ_INTERRUPT=(1<<6),-/* pipeline active */-IDETAPE_FLAG_PIPELINE_ACTIVE=(1<<7),+IDETAPE_FLAG_DRQ_INTERRUPT=(1<<5),/* 0 = no tape is loaded, so we don't rewind after ejecting */-IDETAPE_FLAG_MEDIUM_PRESENT=(1<<8),+IDETAPE_FLAG_MEDIUM_PRESENT=(1<<6),};/* A pipeline stage. */
@@ -2143,19 +2143,6 @@ static void idetape_create_space_cmd(struct ide_atapi_pc *pc, int count, u8 cmd)pc->idetape_callback=&idetape_pc_callback;}-staticvoididetape_wait_first_stage(ide_drive_t*drive)-{-idetape_tape_t*tape=drive->driver_data;-unsignedlongflags;--if(tape->first_stage==NULL)-return;-spin_lock_irqsave(&tape->lock,flags);-if(tape->active_stage==tape->first_stage)-idetape_wait_for_request(drive,tape->active_data_rq);-spin_unlock_irqrestore(&tape->lock,flags);-}-/* Queue up a character device originated write request. */staticintidetape_add_chrdev_write_request(ide_drive_t*drive,intblocks){
@@ -2446,73 +2433,23 @@ static int idetape_blkdev_ioctl(ide_drive_t *drive, unsigned int cmd,return0;}-/*-*Thefunctionbelowisnowabitmorecomplicatedthanjustpassingthe-*commandtothetapesincewemayhavecrossedsomefilemarksduringour-*pipelinedread-aheadmode.Asaminorsideeffect,thepipelineenablesusto-*supportMTFSFMwhenthefilemarkisinourinternalpipelineevenifthetape-*doesn'tsupportspacingoverfilemarksinthereversedirection.-*/staticintidetape_space_over_filemarks(ide_drive_t*drive,shortmt_op,intmt_count){idetape_tape_t*tape=drive->driver_data;structide_atapi_pcpc;-unsignedlongflags;intretval,count=0;intsprev=!!(tape->caps[4]&0x20);if(mt_count==0)return0;+if(MTBSF==mt_op||MTBSFM==mt_op){if(!sprev)return-EIO;mt_count=-mt_count;}-if(tape->chrdev_dir==IDETAPE_DIR_READ){-/* its a read-ahead buffer, scan it for crossed filemarks. */-tape->merge_stage_size=0;-if(test_and_clear_bit(IDETAPE_FLAG_FILEMARK,&tape->flags))-++count;-while(tape->first_stage!=NULL){-if(count==mt_count){-if(mt_op==MTFSFM)-set_bit(IDETAPE_FLAG_FILEMARK,-&tape->flags);-return0;-}-spin_lock_irqsave(&tape->lock,flags);-if(tape->first_stage==tape->active_stage){-/*-*Wehavereachedtheactivestageintheread-*pipeline.Thereisnopointinallowingthe-*drivetocontinuereadinganyfarther,sowe-*stopthepipeline.-*-*Thissectionshouldbemovedtoaseparate-*subroutinebecausesimilaroperationsare-*donein__idetape_discard_read_pipeline(),-*forexample.-*/-tape->next_stage=NULL;-spin_unlock_irqrestore(&tape->lock,flags);-idetape_wait_first_stage(drive);-tape->next_stage=tape->first_stage->next;-}else-spin_unlock_irqrestore(&tape->lock,flags);-if(tape->first_stage->rq.errors==-IDETAPE_ERROR_FILEMARK)-++count;-idetape_remove_stage_head(drive);-}-idetape_discard_read_pipeline(drive,0);-}--/*-*Thefilemarkwasnotfoundinourinternalpipeline;nowwecanissue-*thespacecommand.-*/switch(mt_op){caseMTFSF:caseMTBSF:
Instead of plugging the request into the pipeline, queue it straight on the
device's request queue through idetape_queue_rw_tail().
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 81 ++---------------------------------------------
1 files changed, 4 insertions(+), 77 deletions(-)
@@ -2123,12 +2123,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,debug_log(DBG_SENSE,"%s: cmd=%d\n",__func__,cmd);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-printk(KERN_ERR"ide-tape: bug: the pipeline is active in %s\n",-__func__);-return(0);-}-idetape_init_rq(&rq,cmd);rq.rq_disk=tape->disk;rq.special=(void*)bh;
@@ -2140,10 +2134,9 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,if((cmd&(REQ_IDETAPE_READ|REQ_IDETAPE_WRITE))==0)return0;-if(tape->merge_stage)-idetape_init_merge_stage(tape);if(rq.errors==IDETAPE_ERROR_GENERAL)return-EIO;+return(tape->blk_size*(blocks-rq.current_nr_sectors));}
@@ -2210,81 +2203,15 @@ static void idetape_wait_first_stage(ide_drive_t *drive)spin_unlock_irqrestore(&tape->lock,flags);}-/*-*Trytoaddacharacterdeviceoriginatedwriterequesttoourpipeline.In-*casewedon'tsucceed,wereverttonon-pipelinedoperationmodeforthis-*request.Inordertoaccomplishthat,we-*-*1.Trytoallocateanewpipelinestage.-*2.Ifwecan't,waitformoreandmorerequeststobeservicedandtryagain-*eachtime.-*3.Ifwestillcan'tallocateastage,fallbacktonon-pipelinedoperation-*modeforthisrequest.-*/+/* Queue up a character device originated write request. */staticintidetape_add_chrdev_write_request(ide_drive_t*drive,intblocks){idetape_tape_t*tape=drive->driver_data;-idetape_stage_t*new_stage;-unsignedlongflags;-structrequest*rq;debug_log(DBG_CHRDEV,"Enter %s\n",__func__);-/* Attempt to allocate a new stage. Beware possible race conditions. */-while((new_stage=idetape_kmalloc_stage(tape))==NULL){-spin_lock_irqsave(&tape->lock,flags);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-idetape_wait_for_request(drive,tape->active_data_rq);-spin_unlock_irqrestore(&tape->lock,flags);-}else{-spin_unlock_irqrestore(&tape->lock,flags);-idetape_plug_pipeline(drive);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,-&tape->flags))-continue;-/*-*Themachineisshortonmemory.Fallbacktonon--*pipelinedoperationmodeforthisrequest.-*/-returnidetape_queue_rw_tail(drive,REQ_IDETAPE_WRITE,-blocks,tape->merge_stage->bh);-}-}-rq=&new_stage->rq;-idetape_init_rq(rq,REQ_IDETAPE_WRITE);-/* Doesn't actually matter - We always assume sequential access */-rq->sector=tape->first_frame;-rq->current_nr_sectors=blocks;-rq->nr_sectors=blocks;--idetape_switch_buffers(tape,new_stage);-idetape_add_stage_tail(drive,new_stage);-tape->pipeline_head++;-idetape_calculate_speeds(drive);--/*-*Estimatewhetherthetapehasstoppedwritingbycheckingifour-*writepipelineiscurrentlyempty.Ifwearenotwritinganymore,-*waitforthepipelinetobealmostcompletelyfull(90%)before-*startingtoservicerequests,sothatwewillbeabletokeepupwith-*thehigherspeedsofthetape.-*/-if(!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-if(tape->nr_stages>=tape->max_stages*9/10||-tape->nr_stages>=tape->max_stages--tape->uncontrolled_pipeline_head_speed*3*1024/-tape->blk_size){-tape->measure_insert_time=1;-tape->insert_time=jiffies;-tape->insert_size=0;-tape->insert_speed=0;-idetape_plug_pipeline(drive);-}-}-if(test_and_clear_bit(IDETAPE_FLAG_PIPELINE_ERR,&tape->flags))-/* Return a deferred error */-return-EIO;-returnblocks;+returnidetape_queue_rw_tail(drive,REQ_IDETAPE_WRITE,blocks,+tape->merge_stage->bh);}/*
@@ -8,8 +8,6 @@ * interface, on the other hand, creates new requests, adds them * to the request-list of the block device, and waits for their completion. *- * Pipelined operation mode is now supported on both reads and writes.- * * The block device major and minor numbers are determined from the * tape's relative position in the ide interfaces, as explained in ide.c. *
@@ -45,83 +43,6 @@ * * | Special care is recommended. Have Fun! *- *- * An overview of the pipelined operation mode.- *- * In the pipelined write mode, we will usually just add requests to our- * pipeline and return immediately, before we even start to service them. The- * user program will then have enough time to prepare the next request while- * we are still busy servicing previous requests. In the pipelined read mode,- * the situation is similar - we add read-ahead requests into the pipeline,- * before the user even requested them.- *- * The pipeline can be viewed as a "safety net" which will be activated when- * the system load is high and prevents the user backup program from keeping up- * with the current tape speed. At this point, the pipeline will get- * shorter and shorter but the tape will still be streaming at the same speed.- * Assuming we have enough pipeline stages, the system load will hopefully- * decrease before the pipeline is completely empty, and the backup program- * will be able to "catch up" and refill the pipeline again.- *- * When using the pipelined mode, it would be best to disable any type of- * buffering done by the user program, as ide-tape already provides all the- * benefits in the kernel, where it can be done in a more efficient way.- * As we will usually not block the user program on a request, the most- * efficient user code will then be a simple read-write-read-... cycle.- * Any additional logic will usually just slow down the backup process.- *- * Using the pipelined mode, I get a constant over 400 KBps throughput,- * which seems to be the maximum throughput supported by my tape.- *- * However, there are some downfalls:- *- * 1. We use memory (for data buffers) in proportional to the number- * of pipeline stages (each stage is about 26 KB with my tape).- * 2. In the pipelined write mode, we cheat and postpone error codes- * to the user task. In read mode, the actual tape position- * will be a bit further than the last requested block.- *- * Concerning (1):- *- * 1. We allocate stages dynamically only when we need them. When- * we don't need them, we don't consume additional memory. In- * case we can't allocate stages, we just manage without them- * (at the expense of decreased throughput) so when Linux is- * tight in memory, we will not pose additional difficulties.- *- * 2. The maximum number of stages (which is, in fact, the maximum- * amount of memory) which we allocate is limited by the compile- * time parameter IDETAPE_MAX_PIPELINE_STAGES.- *- * 3. The maximum number of stages is a controlled parameter - We- * don't start from the user defined maximum number of stages- * but from the lower IDETAPE_MIN_PIPELINE_STAGES (again, we- * will not even allocate this amount of stages if the user- * program can't handle the speed). We then implement a feedback- * loop which checks if the pipeline is empty, and if it is, we- * increase the maximum number of stages as necessary until we- * reach the optimum value which just manages to keep the tape- * busy with minimum allocated memory or until we reach- * IDETAPE_MAX_PIPELINE_STAGES.- *- * Concerning (2):- *- * In pipelined write mode, ide-tape can not return accurate error codes- * to the user program since we usually just add the request to the- * pipeline without waiting for it to be serviced. In case an error- * occurs, I will report it on the next user request.- *- * In the pipelined read mode, subsequent read requests or forward- * filemark spacing will perform correctly, as we preserve all blocks- * and filemarks which we encountered during our excess read-ahead.- *- * For accurate tape positioning and error reporting, disabling- * pipelined mode might be the best option.- *- * You can enable/disable/tune the pipelined operation mode by adjusting- * the compile time parameters below.- *- * * Possible improvements. * * 1. Support for the ATAPI overlap protocol.
@@ -267,9 +267,7 @@ typedef struct ide_tape_obj {*WhilepollingforDSCweusepostponed_rqtopostponethecurrent*requestsothatide.cwillbeabletoservicependingrequestsonthe*otherdevice.NotethatatmostwewillhaveonlyoneDSC(usually-*datatransfer)requestinthedevicerequestqueue.Additional-*requestscanbequeuedinourinternalpipeline,buttheywillbe-*visibletoide.conlyoneatatime.+*datatransfer)requestinthedevicerequestqueue.*/structrequest*postponed_rq;/* The time in which we started polling for DSC */
@@ -309,10 +307,8 @@ typedef struct ide_tape_obj {*Atmost,thereisonlyoneide-tapeoriginateddatatransferrequest*inthedevicerequestqueue.Thisallowside.ctoeasilyservice*requestsfromtheotherdevicewhenwepostponeouractiverequest.-*Inthepipelinedoperationmode,weuseourinternalpipeline-*structuretoholdmoredatarequests.Thedatabuffersizeischosen-*basedonthetape'srecommendation.*/+/* ptr to the request which is waiting in the device request queue */structrequest*active_data_rq;/* Data buffer size chosen based on the tape's recommendation */
@@ -1792,8 +1788,7 @@ static int idetape_init_read(ide_drive_t *drive)}/*-*Calledfromidetape_chrdev_read()toserviceacharacterdevicereadrequest-*andaddread-aheadrequeststoourpipeline.+*Calledfromidetape_chrdev_read()toserviceacharacterdevicereadrequest.*/staticintidetape_add_chrdev_read_request(ide_drive_t*drive,intblocks){
@@ -2112,8 +2107,7 @@ static int idetape_write_filemark(ide_drive_t *drive)**Note:MTBSFandMTBSFMarenotsupportedwhenthetapedoesn'tsupport*spacingoverfilemarksinthereversedirection.Inthiscase,MTFSFMisalso-*usuallynotsupported(itissupportedintherarecaseinwhichwecrossed-*thefilemarkduringourread-aheadpipelinedoperationmode).+*usuallynotsupported.**Thefollowingcommandsarecurrentlynotsupported:*
@@ -2129,7 +2123,6 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)debug_log(DBG_ERR,"Handling MTIOCTOP ioctl: mt_op=%d, mt_count=%d\n",mt_op,mt_count);-/* Commands which need our pipelined read-ahead stages. */switch(mt_op){caseMTFSF:caseMTFSFM:
The function used to compute the block_offset in order to write it
into mtget.mt_blkno, among others, by going over the stages still
present in the pipeline and adding the sectors left to submit in each
request. This was being done in idetape_pipeline_size() so remove it.
Since we do non-pipelined operation only, compute the offset now
from the sectors left to submit in the currently active request.
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 39 +++++++++------------------------------
1 files changed, 9 insertions(+), 30 deletions(-)
@@ -2353,27 +2353,6 @@ static void idetape_pad_zeros(ide_drive_t *drive, int bcount)}}-staticintidetape_pipeline_size(ide_drive_t*drive)-{-idetape_tape_t*tape=drive->driver_data;-idetape_stage_t*stage;-structrequest*rq;-intsize=0;--idetape_wait_for_pipeline(drive);-stage=tape->first_stage;-while(stage!=NULL){-rq=&stage->rq;-size+=tape->blk_size*(rq->nr_sectors--rq->current_nr_sectors);-if(rq->errors==IDETAPE_ERROR_FILEMARK)-size+=tape->blk_size;-stage=stage->next;-}-size+=tape->merge_stage_size;-returnsize;-}-/**RewindsthetapetotheBeginningOfthecurrentPartition(BOP).We*currentlysupportonlyonepartition.
@@ -2779,7 +2758,7 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)/**Ourcharacterdeviceioctls.Generalmtio.hmagneticiocommandsare-*supportedhere,andnotinthecorrespondingblockinterface.Ourown+*supportedhereandnotinthecorrespondingblockinterface.Ourown*ide-tapeioctlsaresupportedonbothinterfaces.*/staticintidetape_chrdev_ioctl(structinode*inode,structfile*file,
@@ -1,67 +1,65 @@-/*- * IDE ATAPI streaming tape driver.- *- * This driver is a part of the Linux ide driver.- *- * The driver, in co-operation with ide.c, basically traverses the- * request-list for the block device interface. The character device- * interface, on the other hand, creates new requests, adds them- * to the request-list of the block device, and waits for their completion.- *- * The block device major and minor numbers are determined from the- * tape's relative position in the ide interfaces, as explained in ide.c.- *- * The character device interface consists of the following devices:- *- * ht0 major 37, minor 0 first IDE tape, rewind on close.- * ht1 major 37, minor 1 second IDE tape, rewind on close.- * ...- * nht0 major 37, minor 128 first IDE tape, no rewind on close.- * nht1 major 37, minor 129 second IDE tape, no rewind on close.- * ...- *- * The general magnetic tape commands compatible interface, as defined by- * include/linux/mtio.h, is accessible through the character device.- *- * General ide driver configuration options, such as the interrupt-unmask- * flag, can be configured by issuing an ioctl to the block device interface,- * as any other ide device.- *- * Our own ide-tape ioctl's can be issued to either the block device or- * the character device interface.- *- * Maximal throughput with minimal bus load will usually be achieved in the- * following scenario:- *- * 1. ide-tape is operating in the pipelined operation mode.- * 2. No buffering is performed by the user backup program.- *- * Testing was done with a 2 GB CONNER CTMA 4000 IDE ATAPI Streaming Tape Drive.- *- * Here are some words from the first releases of hd.c, which are quoted- * in ide.c and apply here as well:- *- * | Special care is recommended. Have Fun!- *- * Possible improvements.- *- * 1. Support for the ATAPI overlap protocol.- *- * In order to maximize bus throughput, we currently use the DSC- * overlap method which enables ide.c to service requests from the- * other device while the tape is busy executing a command. The- * DSC overlap method involves polling the tape's status register- * for the DSC bit, and servicing the other device while the tape- * isn't ready.- *- * In the current QIC development standard (December 1995),- * it is recommended that new tape drives will *in addition*- * implement the ATAPI overlap protocol, which is used for the- * same purpose - efficient use of the IDE bus, but is interrupt- * driven and thus has much less CPU overhead.- *- * ATAPI overlap is likely to be supported in most new ATAPI- * devices, including new ATAPI cdroms, and thus provides us- * a method by which we can achieve higher throughput when- * sharing a (fast) ATA-2 disk with any (slow) new ATAPI device.- */+IDE ATAPI streaming tape driver.++This driver is a part of the Linux ide driver.++The driver, in co-operation with ide.c, basically traverses the+request-list for the block device interface. The character device+interface, on the other hand, creates new requests, adds them+to the request-list of the block device, and waits for their completion.++The block device major and minor numbers are determined from the+tape's relative position in the ide interfaces, as explained in ide.c.++The character device interface consists of the following devices:++ht0 major 37, minor 0 first IDE tape, rewind on close.+ht1 major 37, minor 1 second IDE tape, rewind on close.+...+nht0 major 37, minor 128 first IDE tape, no rewind on close.+nht1 major 37, minor 129 second IDE tape, no rewind on close.+...++The general magnetic tape commands compatible interface, as defined by+include/linux/mtio.h, is accessible through the character device.++General ide driver configuration options, such as the interrupt-unmask+flag, can be configured by issuing an ioctl to the block device interface,+as any other ide device.++Our own ide-tape ioctl's can be issued to either the block device or+the character device interface.++Maximal throughput with minimal bus load will usually be achieved in the+following scenario:++ 1. ide-tape is operating in the pipelined operation mode.+ 2. No buffering is performed by the user backup program.++Testing was done with a 2 GB CONNER CTMA 4000 IDE ATAPI Streaming Tape Drive.++Here are some words from the first releases of hd.c, which are quoted+in ide.c and apply here as well:++| Special care is recommended. Have Fun!++Possible improvements:++1. Support for the ATAPI overlap protocol.++In order to maximize bus throughput, we currently use the DSC+overlap method which enables ide.c to service requests from the+other device while the tape is busy executing a command. The+DSC overlap method involves polling the tape's status register+for the DSC bit, and servicing the other device while the tape+isn't ready.++In the current QIC development standard (December 1995),+it is recommended that new tape drives will *in addition*+implement the ATAPI overlap protocol, which is used for the+same purpose - efficient use of the IDE bus, but is interrupt+driven and thus has much less CPU overhead.++ATAPI overlap is likely to be supported in most new ATAPI+devices, including new ATAPI cdroms, and thus provides us+a method by which we can achieve higher throughput when+sharing a (fast) ATA-2 disk with any (slow) new ATAPI device.
Also, strip struct idetape_config down to a single int due to unused/removed
members.
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 17 +++++------------
1 files changed, 5 insertions(+), 12 deletions(-)
@@ -366,7 +366,6 @@ typedef struct ide_tape_obj {/* New requests will be added to the pipeline here */idetape_stage_t*last_stage;/* Optional free stage which we can use */-idetape_stage_t*cache_stage;intpages_per_stage;/* Wasted space in each stage */intexcess_bh_size;
@@ -2045,25 +2044,6 @@ static int idetape_position_tape(ide_drive_t *drive, unsigned int block,return(idetape_queue_pc_tail(drive,&pc));}-staticvoididetape_discard_read_pipeline(ide_drive_t*drive,-intrestore_position)-{-idetape_tape_t*tape=drive->driver_data;-intcnt;-intseek,position;--cnt=__idetape_discard_read_pipeline(drive);-if(restore_position){-position=idetape_read_position(drive);-seek=position>cnt?position-cnt:0;-if(idetape_position_tape(drive,seek,0,0)){-printk(KERN_INFO"ide-tape: %s: position_tape failed in"-" discard_pipeline()\n",tape->name);-return;-}-}-}-/**Generatearead/writerequestfortheblockdeviceinterfaceandwaitforit*tobeserviced.
@@ -2093,19 +2073,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,return(tape->blk_size*(blocks-rq.current_nr_sectors));}-/* start servicing the pipeline stages, starting from tape->next_stage. */-staticvoididetape_plug_pipeline(ide_drive_t*drive)-{-idetape_tape_t*tape=drive->driver_data;--if(tape->next_stage==NULL)-return;-if(!test_and_set_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-idetape_activate_next_stage(drive);-(void)ide_do_drive_cmd(drive,tape->active_data_rq,ide_end);-}-}-staticvoididetape_create_inquiry_cmd(structide_atapi_pc*pc){idetape_init_pc(pc);
@@ -2154,25 +2121,6 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)tape->merge_stage->bh);}-/*-*Waituntilallpendingpipelinerequestsareserviced.Typicallycalledon-*deviceclose.-*/-staticvoididetape_wait_for_pipeline(ide_drive_t*drive)-{-idetape_tape_t*tape=drive->driver_data;-unsignedlongflags;--while(tape->next_stage||test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,-&tape->flags)){-idetape_plug_pipeline(drive);-spin_lock_irqsave(&tape->lock,flags);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags))-idetape_wait_for_request(drive,tape->active_data_rq);-spin_unlock_irqrestore(&tape->lock,flags);-}-}-staticvoididetape_restart_speed_control(ide_drive_t*drive){idetape_tape_t*tape=drive->driver_data;
@@ -2693,7 +2693,6 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)caseMTWEOF:if(tape->write_prot)return-EACCES;-idetape_discard_read_pipeline(drive,1);for(i=0;i<mt_count;i++){retval=idetape_write_filemark(drive);if(retval)
@@ -2701,12 +2700,10 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)}return0;caseMTREW:-idetape_discard_read_pipeline(drive,0);if(idetape_rewind_tape(drive))return-EIO;return0;caseMTLOAD:-idetape_discard_read_pipeline(drive,0);idetape_create_load_unload_cmd(drive,&pc,IDETAPE_LU_LOAD_MASK);returnidetape_queue_pc_tail(drive,&pc);
@@ -2721,7 +2718,6 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)if(!idetape_queue_pc_tail(drive,&pc))tape->door_locked=DOOR_UNLOCKED;}-idetape_discard_read_pipeline(drive,0);idetape_create_load_unload_cmd(drive,&pc,!IDETAPE_LU_LOAD_MASK);retval=idetape_queue_pc_tail(drive,&pc);
@@ -2729,10 +2725,8 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)clear_bit(IDETAPE_FLAG_MEDIUM_PRESENT,&tape->flags);returnretval;caseMTNOP:-idetape_discard_read_pipeline(drive,0);returnidetape_flush_tape_buffers(drive);caseMTRETEN:-idetape_discard_read_pipeline(drive,0);idetape_create_load_unload_cmd(drive,&pc,IDETAPE_LU_RETENSION_MASK|IDETAPE_LU_LOAD_MASK);returnidetape_queue_pc_tail(drive,&pc);
@@ -2754,11 +2748,9 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)set_bit(IDETAPE_FLAG_DETECT_BS,&tape->flags);return0;caseMTSEEK:-idetape_discard_read_pipeline(drive,0);returnidetape_position_tape(drive,mt_count*tape->user_bs_factor,tape->partition,0);caseMTSETPART:-idetape_discard_read_pipeline(drive,0);returnidetape_position_tape(drive,0,mt_count,0);caseMTFSR:caseMTBSR:
@@ -1838,24 +1837,6 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)tape->merge_stage->bh);}-staticvoididetape_restart_speed_control(ide_drive_t*drive)-{-idetape_tape_t*tape=drive->driver_data;--tape->restart_speed_control_req=0;-tape->pipeline_head=0;-tape->controlled_last_pipeline_head=0;-tape->controlled_previous_pipeline_head=0;-tape->uncontrolled_previous_pipeline_head=0;-tape->controlled_pipeline_head_speed=5000;-tape->pipeline_head_speed=5000;-tape->uncontrolled_pipeline_head_speed=0;-tape->controlled_pipeline_head_time=-tape->uncontrolled_pipeline_head_time=jiffies;-tape->controlled_previous_head_time=-tape->uncontrolled_previous_head_time=jiffies;-}-staticintidetape_init_read(ide_drive_t*drive){idetape_tape_t*tape=drive->driver_data;
@@ -2470,9 +2451,6 @@ static int idetape_chrdev_open(struct inode *inode, struct file *filp)if(!test_bit(IDETAPE_FLAG_ADDRESS_VALID,&tape->flags))(void)idetape_rewind_tape(drive);-if(tape->chrdev_dir!=IDETAPE_DIR_READ)-clear_bit(IDETAPE_FLAG_PIPELINE_ERR,&tape->flags);-/* Read block size and write protect status from drive. */ide_tape_get_bsize_from_bdesc(drive);
@@ -2493,16 +2471,12 @@ static int idetape_chrdev_open(struct inode *inode, struct file *filp)}/* Lock the tape drive door so user can't eject. */-if(tape->chrdev_dir==IDETAPE_DIR_NONE){-if(idetape_create_prevent_cmd(drive,&pc,1)){-if(!idetape_queue_pc_tail(drive,&pc)){-if(tape->door_locked!=DOOR_EXPLICITLY_LOCKED)-tape->door_locked=DOOR_LOCKED;-}-}-}-idetape_restart_speed_control(drive);-tape->restart_speed_control_req=0;+if(tape->chrdev_dir==IDETAPE_DIR_NONE&&+idetape_create_prevent_cmd(drive,&pc,1)&&+!idetape_queue_pc_tail(drive,&pc)&&+tape->door_locked!=DOOR_EXPLICITLY_LOCKED)+tape->door_locked=DOOR_LOCKED;+return0;out_put_tape:
@@ -673,28 +673,6 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense)}}-staticvoididetape_activate_next_stage(ide_drive_t*drive)-{-idetape_tape_t*tape=drive->driver_data;-idetape_stage_t*stage=tape->next_stage;-structrequest*rq=&stage->rq;--debug_log(DBG_PROCS,"Enter %s\n",__func__);--if(stage==NULL){-printk(KERN_ERR"ide-tape: bug: Trying to activate a non"-" existing stage\n");-return;-}--rq->rq_disk=tape->disk;-rq->buffer=NULL;-rq->special=(void*)stage->bh;-tape->active_data_rq=rq;-tape->active_stage=stage;-tape->next_stage=stage->next;-}-/* Free a stage along with its related buffers completely. */staticvoid__idetape_kfree_stage(idetape_stage_t*stage){
@@ -717,84 +695,12 @@ static void __idetape_kfree_stage(idetape_stage_t *stage)kfree(stage);}-staticvoididetape_kfree_stage(idetape_tape_t*tape,idetape_stage_t*stage)-{-__idetape_kfree_stage(stage);-}--/*-*Removetape->first_stagefromthepipeline.Thecallershouldavoidrace-*conditions.-*/-staticvoididetape_remove_stage_head(ide_drive_t*drive)-{-idetape_tape_t*tape=drive->driver_data;-idetape_stage_t*stage;--debug_log(DBG_PROCS,"Enter %s\n",__func__);--if(tape->first_stage==NULL){-printk(KERN_ERR"ide-tape: bug: tape->first_stage is NULL\n");-return;-}-if(tape->active_stage==tape->first_stage){-printk(KERN_ERR"ide-tape: bug: Trying to free our active "-"pipeline stage\n");-return;-}-stage=tape->first_stage;-tape->first_stage=stage->next;-idetape_kfree_stage(tape,stage);-tape->nr_stages--;-if(tape->first_stage==NULL){-tape->last_stage=NULL;-if(tape->next_stage!=NULL)-printk(KERN_ERR"ide-tape: bug: tape->next_stage !="-" NULL\n");-if(tape->nr_stages)-printk(KERN_ERR"ide-tape: bug: nr_stages should be 0 "-"now\n");-}-}--/*-*Thiswillfreeallthepipelinestagesstartingfromnew_last_stage->next-*totheendofthelist,andpointtape->last_stagetonew_last_stage.-*/-staticvoididetape_abort_pipeline(ide_drive_t*drive,-idetape_stage_t*new_last_stage)-{-idetape_tape_t*tape=drive->driver_data;-idetape_stage_t*stage=new_last_stage->next;-idetape_stage_t*nstage;--debug_log(DBG_PROCS,"%s: Enter %s\n",tape->name,__func__);--while(stage){-nstage=stage->next;-idetape_kfree_stage(tape,stage);---tape->nr_stages;---tape->nr_pending_stages;-stage=nstage;-}-if(new_last_stage)-new_last_stage->next=NULL;-tape->last_stage=new_last_stage;-tape->next_stage=NULL;-}--/*-*Finishservicingarequestandinsertapendingpipelinerequestintothe-*maindevicequeue.-*/+/* Finish servicing a request. */staticintidetape_end_request(ide_drive_t*drive,intuptodate,intnr_sects){structrequest*rq=HWGROUP(drive)->rq;idetape_tape_t*tape=drive->driver_data;-unsignedlongflags;interror;-intremove_stage=0;-idetape_stage_t*active_stage;debug_log(DBG_PROCS,"Enter %s\n",__func__);
@@ -812,61 +718,8 @@ static int idetape_end_request(ide_drive_t *drive, int uptodate, int nr_sects)return0;}-spin_lock_irqsave(&tape->lock,flags);--/* The request was a pipelined data transfer request */-if(tape->active_data_rq==rq){-active_stage=tape->active_stage;-tape->active_stage=NULL;-tape->active_data_rq=NULL;-tape->nr_pending_stages--;-if(rq->cmd[0]&REQ_IDETAPE_WRITE){-remove_stage=1;-if(error){-set_bit(IDETAPE_FLAG_PIPELINE_ERR,-&tape->flags);-if(error==IDETAPE_ERROR_EOD)-idetape_abort_pipeline(drive,-active_stage);-}-}elseif(rq->cmd[0]&REQ_IDETAPE_READ){-if(error==IDETAPE_ERROR_EOD){-set_bit(IDETAPE_FLAG_PIPELINE_ERR,-&tape->flags);-idetape_abort_pipeline(drive,active_stage);-}-}-if(tape->next_stage!=NULL){-idetape_activate_next_stage(drive);--/* Insert the next request into the request queue. */-(void)ide_do_drive_cmd(drive,tape->active_data_rq,-ide_end);-}elseif(!error){-/*-*Thisisapartofthefeedbackloopwhichtriesto-*findtheoptimumnumberofstages.Wearestarting-*fromaminimummaximumnumberofstages,andifwe-*sensethatthepipelineisempty,wetrytoincrease-*it,untilwereachtheusercompiletimememory-*limit.-*/-inti=(tape->max_pipeline-tape->min_pipeline)/10;--tape->max_stages+=max(i,1);-tape->max_stages=max(tape->max_stages,-tape->min_pipeline);-tape->max_stages=min(tape->max_stages,-tape->max_pipeline);-}-}ide_end_drive_cmd(drive,0,0);-if(remove_stage)-idetape_remove_stage_head(drive);-if(tape->active_data_rq==NULL)-clear_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags);-spin_unlock_irqrestore(&tape->lock,flags);return0;}
@@ -1760,29 +1613,6 @@ static void idetape_init_merge_stage(idetape_tape_t *tape)}}-/*-*Installacompletioninapendingrequestandsleepuntilitisserviced.The-*callershouldensurethattherequestwillnotbeservicedbeforeweinstall-*thecompletion(usuallybydisablinginterrupts).-*/-staticvoididetape_wait_for_request(ide_drive_t*drive,structrequest*rq)-{-DECLARE_COMPLETION_ONSTACK(wait);-idetape_tape_t*tape=drive->driver_data;--if(rq==NULL||!blk_special_request(rq)){-printk(KERN_ERR"ide-tape: bug: Trying to sleep on non-valid"-" request\n");-return;-}-rq->end_io_data=&wait;-rq->end_io=blk_end_sync_rq;-spin_unlock_irq(&tape->lock);-wait_for_completion(&wait);-/* The stage and its struct request have been deallocated */-spin_lock_irq(&tape->lock);-}-staticide_startstop_tidetape_read_position_callback(ide_drive_t*drive){idetape_tape_t*tape=drive->driver_data;
@@ -1776,30 +1761,8 @@ static void idetape_init_merge_stage(idetape_tape_t *tape)}}-/* Add a new stage at the end of the pipeline. */-staticvoididetape_add_stage_tail(ide_drive_t*drive,idetape_stage_t*stage)-{-idetape_tape_t*tape=drive->driver_data;-unsignedlongflags;--debug_log(DBG_PROCS,"Enter %s\n",__func__);--spin_lock_irqsave(&tape->lock,flags);-stage->next=NULL;-if(tape->last_stage!=NULL)-tape->last_stage->next=stage;-else-tape->first_stage=stage;-tape->next_stage=stage;-tape->last_stage=stage;-if(tape->next_stage==NULL)-tape->next_stage=tape->last_stage;-tape->nr_stages++;-tape->nr_pending_stages++;-spin_unlock_irqrestore(&tape->lock,flags);-}--/* Install a completion in a pending request and sleep until it is serviced. The+/*+*Installacompletioninapendingrequestandsleepuntilitisserviced.The*callershouldensurethattherequestwillnotbeservicedbeforeweinstall*thecompletion(usuallybydisablinginterrupts).*/
@@ -2347,8 +2305,8 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)*/if(drive->dsc_overlap){bytes_read=idetape_queue_rw_tail(drive,-REQ_IDETAPE_READ,0,-tape->merge_stage->bh);+REQ_IDETAPE_READ,0,+tape->merge_stage->bh);if(bytes_read<0){__idetape_kfree_stage(tape->merge_stage);tape->merge_stage=NULL;
@@ -2357,32 +2315,11 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)}}}-if(tape->restart_speed_control_req)-idetape_restart_speed_control(drive);idetape_init_rq(&rq,REQ_IDETAPE_READ);rq.sector=tape->first_frame;rq.nr_sectors=blocks;rq.current_nr_sectors=blocks;-if(!test_bit(IDETAPE_FLAG_PIPELINE_ERR,&tape->flags)&&-tape->nr_stages<max_stages){-new_stage=idetape_kmalloc_stage(tape);-while(new_stage!=NULL){-new_stage->rq=rq;-idetape_add_stage_tail(drive,new_stage);-if(tape->nr_stages>=max_stages)-break;-new_stage=idetape_kmalloc_stage(tape);-}-}-if(!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-if(tape->nr_pending_stages>=3*max_stages/4){-tape->measure_insert_time=1;-tape->insert_time=jiffies;-tape->insert_size=0;-tape->insert_speed=0;-idetape_plug_pipeline(drive);-}-}+return0;}
Most of them have become unused after removing the pipelined
functionality - the rest is only being written to.
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 95 +-----------------------------------------------
1 files changed, 2 insertions(+), 93 deletions(-)
@@ -323,26 +323,6 @@ typedef struct ide_tape_obj {char*b_data;intb_count;-/*-*Pipelineparameters.-*-*Toaccomplishnon-pipelinedmode,wesimplysetthefollowing-*variablestozero(orNULL,whereappropriate).-*/-/* Number of currently used stages */-intnr_stages;-/* Number of pending stages */-intnr_pending_stages;-/* We will not allocate more than this number of stages */-intmax_stages,min_pipeline,max_pipeline;-/* The first stage which will be removed from the pipeline */-idetape_stage_t*first_stage;-/* The currently active stage */-idetape_stage_t*active_stage;-/* Will be serviced after the currently active request */-idetape_stage_t*next_stage;-/* New requests will be added to the pipeline here */-idetape_stage_t*last_stage;/* Optional free stage which we can use */intpages_per_stage;/* Wasted space in each stage */
@@ -365,43 +345,8 @@ typedef struct ide_tape_obj {/* the tape is write protected (hardware or opened as read-only) */charwrite_prot;-/*-*Limitthenumberoftimesarequestcanbepostponed,toavoidan-*infinitepostponedeadlock.-*/-intpostpone_cnt;--/*-*Measuresnumberofframes:-*-*1.written/readto/fromthedriverpipeline(pipeline_head).-*2.written/readto/fromthetapebuffers(idetape_bh).-*3.written/readbythetapeto/fromthemedia(tape_head).-*/-intpipeline_head;-intbuffer_head;-inttape_head;-intlast_tape_head;--/* Speed control at the tape buffers input/output */-unsignedlonginsert_time;+/* size control at the tape buffers input/output */intinsert_size;-intinsert_speed;-intmax_insert_speed;-intmeasure_insert_time;--/* Speed regulation negative feedback loop */-intspeed_control;-intpipeline_head_speed;-intcontrolled_pipeline_head_speed;-intuncontrolled_pipeline_head_speed;-intcontrolled_last_pipeline_head;-unsignedlonguncontrolled_pipeline_head_time;-unsignedlongcontrolled_pipeline_head_time;-intcontrolled_previous_pipeline_head;-intuncontrolled_previous_pipeline_head;-unsignedlongcontrolled_previous_head_time;-unsignedlonguncontrolled_previous_head_time;u32debug_mask;}idetape_tape_t;
This function was simply a wrapper for a test_bit() macro so remove it and
use the macro instead.
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 29 +++++++++++------------------
1 files changed, 11 insertions(+), 18 deletions(-)
@@ -1605,14 +1605,6 @@ out:}/* Pipeline related functions */-staticinlineintidetape_pipeline_active(idetape_tape_t*tape)-{-intrc1,rc2;--rc1=test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags);-rc2=(tape->active_data_rq!=NULL);-returnrc1;-}/**Thefunctionbelowuses__get_free_pagetoallocateapipelinestage,along
@@ -2058,7 +2050,7 @@ static int __idetape_discard_read_pipeline(ide_drive_t *drive)spin_lock_irqsave(&tape->lock,flags);tape->next_stage=NULL;-if(idetape_pipeline_active(tape))+if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags))idetape_wait_for_request(drive,tape->active_data_rq);spin_unlock_irqrestore(&tape->lock,flags);
@@ -2131,7 +2123,7 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,debug_log(DBG_SENSE,"%s: cmd=%d\n",__func__,cmd);-if(idetape_pipeline_active(tape)){+if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){printk(KERN_ERR"ide-tape: bug: the pipeline is active in %s\n",__func__);return(0);
@@ -2242,13 +2233,14 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)/* Attempt to allocate a new stage. Beware possible race conditions. */while((new_stage=idetape_kmalloc_stage(tape))==NULL){spin_lock_irqsave(&tape->lock,flags);-if(idetape_pipeline_active(tape)){+if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){idetape_wait_for_request(drive,tape->active_data_rq);spin_unlock_irqrestore(&tape->lock,flags);}else{spin_unlock_irqrestore(&tape->lock,flags);idetape_plug_pipeline(drive);-if(idetape_pipeline_active(tape))+if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,+&tape->flags))continue;/**Themachineisshortonmemory.Fallbacktonon-
@@ -2277,7 +2269,7 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)*startingtoservicerequests,sothatwewillbeabletokeepupwith*thehigherspeedsofthetape.*/-if(!idetape_pipeline_active(tape)){+if(!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){if(tape->nr_stages>=tape->max_stages*9/10||tape->nr_stages>=tape->max_stages-tape->uncontrolled_pipeline_head_speed*3*1024/
@@ -2464,7 +2457,7 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)new_stage=idetape_kmalloc_stage(tape);}}-if(!idetape_pipeline_active(tape)){+if(!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){if(tape->nr_pending_stages>=3*max_stages/4){tape->measure_insert_time=1;tape->insert_time=jiffies;
We fall back to non-pipelined operation through idetape_queue_rw_tail with cmd
set to REQ_IDETAPE_READ. Also, remove idetape_switch_buffers() since it becomes
unused.
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 46 +---------------------------------------------
1 files changed, 1 insertions(+), 45 deletions(-)
@@ -1776,16 +1776,6 @@ static void idetape_init_merge_stage(idetape_tape_t *tape)}}-staticvoididetape_switch_buffers(idetape_tape_t*tape,idetape_stage_t*stage)-{-structidetape_bh*tmp;--tmp=stage->bh;-stage->bh=tape->merge_stage->bh;-tape->merge_stage->bh=tmp;-idetape_init_merge_stage(tape);-}-/* Add a new stage at the end of the pipeline. */staticvoididetape_add_stage_tail(ide_drive_t*drive,idetape_stage_t*stage){
@@ -2403,9 +2393,6 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)staticintidetape_add_chrdev_read_request(ide_drive_t*drive,intblocks){idetape_tape_t*tape=drive->driver_data;-unsignedlongflags;-structrequest*rq_ptr;-intbytes_read;debug_log(DBG_PROCS,"Enter %s, %d blocks\n",__func__,blocks);
@@ -2413,39 +2400,8 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)if(test_bit(IDETAPE_FLAG_FILEMARK,&tape->flags))return0;-/* Wait for the next block to reach the head of the pipeline. */-idetape_init_read(drive,tape->max_stages);-if(tape->first_stage==NULL){-if(test_bit(IDETAPE_FLAG_PIPELINE_ERR,&tape->flags))-return0;-returnidetape_queue_rw_tail(drive,REQ_IDETAPE_READ,blocks,+returnidetape_queue_rw_tail(drive,REQ_IDETAPE_READ,blocks,tape->merge_stage->bh);-}-idetape_wait_first_stage(drive);-rq_ptr=&tape->first_stage->rq;-bytes_read=tape->blk_size*(rq_ptr->nr_sectors--rq_ptr->current_nr_sectors);-rq_ptr->nr_sectors=0;-rq_ptr->current_nr_sectors=0;--if(rq_ptr->errors==IDETAPE_ERROR_EOD)-return0;-else{-idetape_switch_buffers(tape,tape->first_stage);-if(rq_ptr->errors==IDETAPE_ERROR_FILEMARK)-set_bit(IDETAPE_FLAG_FILEMARK,&tape->flags);-spin_lock_irqsave(&tape->lock,flags);-idetape_remove_stage_head(drive);-spin_unlock_irqrestore(&tape->lock,flags);-tape->pipeline_head++;-idetape_calculate_speeds(drive);-}-if(bytes_read>blocks*tape->blk_size){-printk(KERN_ERR"ide-tape: bug: trying to return more bytes"-" than requested\n");-bytes_read=blocks*tape->blk_size;-}-return(bytes_read);}staticvoididetape_pad_zeros(ide_drive_t*drive,intbcount)
Hi Bart,
here's the 1st draft of the pipeline removal series. As the diffstat below openly
states it, a lot of code got removed - even more than the cleanup series we did
earlier. There are several issues that we need to address concerning these
patches:
1. only compile-tested since i don't have the hardware, i.e. longer -mm brewing is
advisable the least.
2. I have left the tape->merge_stage buffer structure along with its
alloc/free functions intact for now, for simplicity. The next step would be
to go and carefully audit the code and then remove that last piece
too and use allocations on the stack instead. I guess we still expect
Jens's response on whether blk_{get,put}_request is the way to go here.
Jens?
Hm, I have not seen any questions regarding this directed my way :-)
Please point me to the original question and I'll take a look at it.
--
Jens Axboe
From: Adrian Bunk <bunk@kernel.org> Date: 2008-03-01 10:22:17
On Sat, Mar 01, 2008 at 09:58:24AM +0100, Borislav Petkov wrote:
Hi Bart,
here's the 1st draft of the pipeline removal series. As the diffstat below openly
states it, a lot of code got removed - even more than the cleanup series we did
earlier.
...
After seeing commit d48567dd43868b3d2e1fcc33ee76dc2d38a1ddeb that
schedules ide-tape for removal I'm wondering whether any cleanups of
this driver make any sense at all:
What: ide-tape driver
When: July 2008
Files: drivers/ide/ide-tape.c
Why: This driver might not have any users anymore and maintaining it for no
reason is an effort no one wants to make.
Who: Bartlomiej Zolnierkiewicz [off-list ref], Borislav Petkov
[off-list ref]
Your patches will go in 2.6.26 and the driver will be removed
in 2.6.27 ...
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Sat, Mar 01, 2008 at 12:20:38PM +0200, Adrian Bunk wrote:
On Sat, Mar 01, 2008 at 09:58:24AM +0100, Borislav Petkov wrote:
quoted
Hi Bart,
here's the 1st draft of the pipeline removal series. As the diffstat below openly
states it, a lot of code got removed - even more than the cleanup series we did
earlier.
...
After seeing commit d48567dd43868b3d2e1fcc33ee76dc2d38a1ddeb that
schedules ide-tape for removal I'm wondering whether any cleanups of
this driver make any sense at all:
Hi Adrian,
you're right and I'm expecting Bart's input on that. However, removing
tape support in ide altogether is probably not the right thing to do and
Bart wanted to keep some kind of a basic, ide-tape "light" version in so,
yes, the above commit is misleading.
Here's a fix, Bart please apply.
--
From 11c41d7760dd0b8f4cd1ab3076c86a2c4beec4de Mon Sep 17 00:00:00 2001
From: Borislav Petkov <redacted>
Date: Sat, 1 Mar 2008 16:31:17 +0100
Subject: [PATCH] ide-tape: keep a light version in ide tree
Keep a light version of the driver in for the
small amount of ide tape hardware still using it.
Signed-off-by: Borislav Petkov <redacted>
---
Documentation/feature-removal-schedule.txt | 10 ----------
1 files changed, 0 insertions(+), 10 deletions(-)
@@ -172,16 +172,6 @@ Who: Len Brown <len.brown@intel.com> ----------------------------What: ide-tape driver-When: July 2008-Files: drivers/ide/ide-tape.c-Why: This driver might not have any users anymore and maintaining it for no- reason is an effort no one wants to make.-Who: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>, Borislav Petkov- <petkovbb@googlemail.com>------------------------------ What: libata spindown skipping and warning When: Dec 2008 Why: Some halt(8) implementations synchronize caches for and spin
On Sat, Mar 01, 2008 at 10:55:18AM +0100, Jens Axboe wrote:
On Sat, Mar 01 2008, Borislav Petkov wrote:
quoted
Hi Bart,
here's the 1st draft of the pipeline removal series. As the diffstat below openly
states it, a lot of code got removed - even more than the cleanup series we did
earlier. There are several issues that we need to address concerning these
patches:
1. only compile-tested since i don't have the hardware, i.e. longer -mm brewing is
advisable the least.
2. I have left the tape->merge_stage buffer structure along with its
alloc/free functions intact for now, for simplicity. The next step would be
to go and carefully audit the code and then remove that last piece
too and use allocations on the stack instead. I guess we still expect
Jens's response on whether blk_{get,put}_request is the way to go here.
Jens?
Hm, I have not seen any questions regarding this directed my way :-)
Please point me to the original question and I'll take a look at it.
Hi Jens,
sorry but maybe we weren't that explicit, here's a pointer to the relevant
thread: http://www.mail-archive.com/linux-ide@vger.kernel.org/msg15541.html. It
boils down to removing the statically allocated arrays of buffers for pc and rq
structs in ide-floppy and ide-tape, and using GFP_ATOMIC stack memory instead.
Bart's idea was to even go a step further and even avoid allocation errors in
out-of-mem situations by reusing requests from the request queue but wasn't sure
whether this'll fly and wanted to run it by you...
Thanks.
--
Regards/Gruß,
Boris.
On Sat, Mar 01, 2008 at 10:55:18AM +0100, Jens Axboe wrote:
quoted
On Sat, Mar 01 2008, Borislav Petkov wrote:
quoted
Hi Bart,
here's the 1st draft of the pipeline removal series. As the diffstat below openly
states it, a lot of code got removed - even more than the cleanup series we did
earlier. There are several issues that we need to address concerning these
patches:
1. only compile-tested since i don't have the hardware, i.e. longer -mm brewing is
advisable the least.
2. I have left the tape->merge_stage buffer structure along with its
alloc/free functions intact for now, for simplicity. The next step would be
to go and carefully audit the code and then remove that last piece
too and use allocations on the stack instead. I guess we still expect
Jens's response on whether blk_{get,put}_request is the way to go here.
Jens?
Hm, I have not seen any questions regarding this directed my way :-)
Please point me to the original question and I'll take a look at it.
Hi Jens,
sorry but maybe we weren't that explicit, here's a pointer to the
relevant thread:
http://www.mail-archive.com/linux-ide@vger.kernel.org/msg15541.html.
It boils down to removing the statically allocated arrays of buffers
for pc and rq structs in ide-floppy and ide-tape, and using GFP_ATOMIC
stack memory instead. Bart's idea was to even go a step further and
even avoid allocation errors in out-of-mem situations by reusing
requests from the request queue but wasn't sure whether this'll fly
and wanted to run it by you...
That sounds like asking for trouble, if you ask me (well you did :-)
And being a very rarely exercised path (if at all, you should be very
unlucky to NOT get a request even with GFP_ATOMIC), then it wont even
get tested properly. So I'd say stick to GFP_ATOMIC, and just plug the
device and retry if you get into allocation errors.
--
Jens Axboe
Instead of plugging the request into the pipeline, queue it straight on the
device's request queue through idetape_queue_rw_tail().
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 81 ++---------------------------------------------
1 files changed, 4 insertions(+), 77 deletions(-)
@@ -2123,12 +2123,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,debug_log(DBG_SENSE,"%s: cmd=%d\n",__func__,cmd);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-printk(KERN_ERR"ide-tape: bug: the pipeline is active in %s\n",-__func__);-return(0);-}-idetape_init_rq(&rq,cmd);rq.rq_disk=tape->disk;rq.special=(void*)bh;
@@ -2140,10 +2134,9 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,if((cmd&(REQ_IDETAPE_READ|REQ_IDETAPE_WRITE))==0)return0;-if(tape->merge_stage)-idetape_init_merge_stage(tape);if(rq.errors==IDETAPE_ERROR_GENERAL)return-EIO;+return(tape->blk_size*(blocks-rq.current_nr_sectors));}
These two changes to idetape_queue_rw_tail() don't look correct
as the pipeline mode is still used by read requests.
quoted hunk
@@ -2210,81 +2203,15 @@ static void idetape_wait_first_stage(ide_drive_t *drive) spin_unlock_irqrestore(&tape->lock, flags); }-/*- * Try to add a character device originated write request to our pipeline. In- * case we don't succeed, we revert to non-pipelined operation mode for this- * request. In order to accomplish that, we- *- * 1. Try to allocate a new pipeline stage.- * 2. If we can't, wait for more and more requests to be serviced and try again- * each time.- * 3. If we still can't allocate a stage, fallback to non-pipelined operation- * mode for this request.- */+/* Queue up a character device originated write request. */ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks) { idetape_tape_t *tape = drive->driver_data;- idetape_stage_t *new_stage;- unsigned long flags;- struct request *rq; debug_log(DBG_CHRDEV, "Enter %s\n", __func__);- /* Attempt to allocate a new stage. Beware possible race conditions. */- while ((new_stage = idetape_kmalloc_stage(tape)) == NULL) {- spin_lock_irqsave(&tape->lock, flags);- if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {- idetape_wait_for_request(drive, tape->active_data_rq);- spin_unlock_irqrestore(&tape->lock, flags);- } else {- spin_unlock_irqrestore(&tape->lock, flags);- idetape_plug_pipeline(drive);- if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,- &tape->flags))- continue;
Can all the above code be safely removed (are you sure that there are no
hidden interactions)? Even if so I would prefer that it is left intact by
this patch to ease the review.
The main change to idetape_add_chrdev_write_request is fine.
We fall back to non-pipelined operation through idetape_queue_rw_tail with cmd
set to REQ_IDETAPE_READ. Also, remove idetape_switch_buffers() since it becomes
unused.
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 46 +---------------------------------------------
1 files changed, 1 insertions(+), 45 deletions(-)
@@ -1776,16 +1776,6 @@ static void idetape_init_merge_stage(idetape_tape_t *tape)}}-staticvoididetape_switch_buffers(idetape_tape_t*tape,idetape_stage_t*stage)-{-structidetape_bh*tmp;--tmp=stage->bh;-stage->bh=tape->merge_stage->bh;-tape->merge_stage->bh=tmp;-idetape_init_merge_stage(tape);-}-/* Add a new stage at the end of the pipeline. */staticvoididetape_add_stage_tail(ide_drive_t*drive,idetape_stage_t*stage){
@@ -2403,9 +2393,6 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)staticintidetape_add_chrdev_read_request(ide_drive_t*drive,intblocks){idetape_tape_t*tape=drive->driver_data;-unsignedlongflags;-structrequest*rq_ptr;-intbytes_read;debug_log(DBG_PROCS,"Enter %s, %d blocks\n",__func__,blocks);
@@ -2413,39 +2400,8 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)if(test_bit(IDETAPE_FLAG_FILEMARK,&tape->flags))return0;-/* Wait for the next block to reach the head of the pipeline. */-idetape_init_read(drive,tape->max_stages);
Can it be simply removed? Why?
quoted hunk
- if (tape->first_stage == NULL) {- if (test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))- return 0;- return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,+ return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks, tape->merge_stage->bh);
Looking at the driver code tape->first_stage is not always NULL,
seems like ide_tape_add_stage_tail() should vanish first?
please move it at the beginning of the patch series
Lets stop at this patch for now and get changes from patches #2-6
into IDE tree first before moving on patches #7-24.
Thanks,
Bart
On Sun, Mar 02, 2008 at 07:33:05PM +0100, Bartlomiej Zolnierkiewicz wrote:
On Saturday 01 March 2008, Borislav Petkov wrote:
quoted
Instead of plugging the request into the pipeline, queue it straight on the
device's request queue through idetape_queue_rw_tail().
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 81 ++---------------------------------------------
1 files changed, 4 insertions(+), 77 deletions(-)
@@ -2123,12 +2123,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,debug_log(DBG_SENSE,"%s: cmd=%d\n",__func__,cmd);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-printk(KERN_ERR"ide-tape: bug: the pipeline is active in %s\n",-__func__);-return(0);-}-idetape_init_rq(&rq,cmd);rq.rq_disk=tape->disk;rq.special=(void*)bh;
@@ -2140,10 +2134,9 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,if((cmd&(REQ_IDETAPE_READ|REQ_IDETAPE_WRITE))==0)return0;-if(tape->merge_stage)-idetape_init_merge_stage(tape);if(rq.errors==IDETAPE_ERROR_GENERAL)return-EIO;+return(tape->blk_size*(blocks-rq.current_nr_sectors));}
These two changes to idetape_queue_rw_tail() don't look correct
as the pipeline mode is still used by read requests.
Wrt first hunk read rq pipeline functionality is removed in the following
patch. Would it then be better to merge the two patches? Actually, do we need
to keep the driver functional in between the patches of those series for
the purposes of git bisection or only compile-testing it is enough? Cause
after applying the whole series you get pipelined mode ripped out anyway and
intermittent states with partially functional pipeline are of no importance, no?
Wrt the second one you're right, this one should stay in for now until
tape->merge_stage has been removed.
quoted
@@ -2210,81 +2203,15 @@ static void idetape_wait_first_stage(ide_drive_t *drive) spin_unlock_irqrestore(&tape->lock, flags); }-/*- * Try to add a character device originated write request to our pipeline. In- * case we don't succeed, we revert to non-pipelined operation mode for this- * request. In order to accomplish that, we- *- * 1. Try to allocate a new pipeline stage.- * 2. If we can't, wait for more and more requests to be serviced and try again- * each time.- * 3. If we still can't allocate a stage, fallback to non-pipelined operation- * mode for this request.- */+/* Queue up a character device originated write request. */ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks) { idetape_tape_t *tape = drive->driver_data;- idetape_stage_t *new_stage;- unsigned long flags;- struct request *rq; debug_log(DBG_CHRDEV, "Enter %s\n", __func__);- /* Attempt to allocate a new stage. Beware possible race conditions. */- while ((new_stage = idetape_kmalloc_stage(tape)) == NULL) {- spin_lock_irqsave(&tape->lock, flags);- if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {- idetape_wait_for_request(drive, tape->active_data_rq);- spin_unlock_irqrestore(&tape->lock, flags);- } else {- spin_unlock_irqrestore(&tape->lock, flags);- idetape_plug_pipeline(drive);- if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,- &tape->flags))- continue;
Can all the above code be safely removed (are you sure that there are no
hidden interactions)? Even if so I would prefer that it is left intact by
this patch to ease the review.
This code does exactly what the comment above explains: it tries to free
the pipeline for yet another request by plugging it with the already queued
ones and if it can't do so it simply queues the request in non-pipelined
mode. What the patch does is remove the plugging and waiting. If we
leave this intact we'll be missing the point of the whole exercise.
--
Regards/Gruß,
Boris.
On Sun, Mar 02, 2008 at 07:36:12PM +0100, Bartlomiej Zolnierkiewicz wrote:
On Saturday 01 March 2008, Borislav Petkov wrote:
quoted
We fall back to non-pipelined operation through idetape_queue_rw_tail with cmd
set to REQ_IDETAPE_READ. Also, remove idetape_switch_buffers() since it becomes
unused.
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 46 +---------------------------------------------
1 files changed, 1 insertions(+), 45 deletions(-)
@@ -1776,16 +1776,6 @@ static void idetape_init_merge_stage(idetape_tape_t *tape)}}-staticvoididetape_switch_buffers(idetape_tape_t*tape,idetape_stage_t*stage)-{-structidetape_bh*tmp;--tmp=stage->bh;-stage->bh=tape->merge_stage->bh;-tape->merge_stage->bh=tmp;-idetape_init_merge_stage(tape);-}-/* Add a new stage at the end of the pipeline. */staticvoididetape_add_stage_tail(ide_drive_t*drive,idetape_stage_t*stage){
@@ -2403,9 +2393,6 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)staticintidetape_add_chrdev_read_request(ide_drive_t*drive,intblocks){idetape_tape_t*tape=drive->driver_data;-unsignedlongflags;-structrequest*rq_ptr;-intbytes_read;debug_log(DBG_PROCS,"Enter %s, %d blocks\n",__func__,blocks);
@@ -2413,39 +2400,8 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)if(test_bit(IDETAPE_FLAG_FILEMARK,&tape->flags))return0;-/* Wait for the next block to reach the head of the pipeline. */-idetape_init_read(drive,tape->max_stages);
Can it be simply removed? Why?
Bugger, actually this one _has_ to stay since it inits the tape->merge_stage
buffer.
quoted
- if (tape->first_stage == NULL) {- if (test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))- return 0;- return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,+ return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks, tape->merge_stage->bh);
Looking at the driver code tape->first_stage is not always NULL,
seems like ide_tape_add_stage_tail() should vanish first?
At this moment there are no more stages in the pipeline besides only
tape->merge_stage. But this boils down to whether we want to keep the driver
functional in intermittent stages of the removal, as i pointed out before...
--
Regards/Gruß,
Boris.
On Sun, Mar 02, 2008 at 07:33:05PM +0100, Bartlomiej Zolnierkiewicz wrote:
quoted
On Saturday 01 March 2008, Borislav Petkov wrote:
quoted
Instead of plugging the request into the pipeline, queue it straight on the
device's request queue through idetape_queue_rw_tail().
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 81 ++---------------------------------------------
1 files changed, 4 insertions(+), 77 deletions(-)
@@ -2123,12 +2123,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,debug_log(DBG_SENSE,"%s: cmd=%d\n",__func__,cmd);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-printk(KERN_ERR"ide-tape: bug: the pipeline is active in %s\n",-__func__);-return(0);-}-idetape_init_rq(&rq,cmd);rq.rq_disk=tape->disk;rq.special=(void*)bh;
@@ -2140,10 +2134,9 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,if((cmd&(REQ_IDETAPE_READ|REQ_IDETAPE_WRITE))==0)return0;-if(tape->merge_stage)-idetape_init_merge_stage(tape);if(rq.errors==IDETAPE_ERROR_GENERAL)return-EIO;+return(tape->blk_size*(blocks-rq.current_nr_sectors));}
These two changes to idetape_queue_rw_tail() don't look correct
as the pipeline mode is still used by read requests.
Wrt first hunk read rq pipeline functionality is removed in the following
patch. Would it then be better to merge the two patches? Actually, do we need
Merging patches together would cause increased complexity.
The best solution would be to move this hunk to the patch which removes
IDETAPE_FLAG_PIPELINE_ACTIVE flag.
to keep the driver functional in between the patches of those series for
the purposes of git bisection or only compile-testing it is enough? Cause
We want to keep the driver functional in between the patches, especially
given that it shouldn't be much more difficult to do so.
after applying the whole series you get pipelined mode ripped out anyway and
intermittent states with partially functional pipeline are of no importance, no?
We always want fully bisectable patches:
- if the patch series accidentaly completely breaks the code we want to be
able narrow it down to the guilty change
- if the patch series causes some regressions (ie. worse performance) we
also want to see which exact change caused it
[ Nothing changes here and removal of pipeline functionality can't be an
excuse for not sticking to the standard procedure. ]
Wrt the second one you're right, this one should stay in for now until
tape->merge_stage has been removed.
quoted
quoted
@@ -2210,81 +2203,15 @@ static void idetape_wait_first_stage(ide_drive_t *drive) spin_unlock_irqrestore(&tape->lock, flags); }-/*- * Try to add a character device originated write request to our pipeline. In- * case we don't succeed, we revert to non-pipelined operation mode for this- * request. In order to accomplish that, we- *- * 1. Try to allocate a new pipeline stage.- * 2. If we can't, wait for more and more requests to be serviced and try again- * each time.- * 3. If we still can't allocate a stage, fallback to non-pipelined operation- * mode for this request.- */+/* Queue up a character device originated write request. */ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks) { idetape_tape_t *tape = drive->driver_data;- idetape_stage_t *new_stage;- unsigned long flags;- struct request *rq; debug_log(DBG_CHRDEV, "Enter %s\n", __func__);- /* Attempt to allocate a new stage. Beware possible race conditions. */- while ((new_stage = idetape_kmalloc_stage(tape)) == NULL) {- spin_lock_irqsave(&tape->lock, flags);- if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {- idetape_wait_for_request(drive, tape->active_data_rq);- spin_unlock_irqrestore(&tape->lock, flags);- } else {- spin_unlock_irqrestore(&tape->lock, flags);- idetape_plug_pipeline(drive);- if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,- &tape->flags))- continue;
Can all the above code be safely removed (are you sure that there are no
hidden interactions)? Even if so I would prefer that it is left intact by
this patch to ease the review.
This code does exactly what the comment above explains: it tries to free
the pipeline for yet another request by plugging it with the already queued
ones and if it can't do so it simply queues the request in non-pipelined
mode. What the patch does is remove the plugging and waiting. If we
leave this intact we'll be missing the point of the whole exercise.
idetape_empty_write_pipeline() which is called for _reads_ calls
idetape_add_chrdev_write_request() - could you tell if there are none
hidden interactions caused by not waiting for the active request to
finish?
[ I don't know and it is really not obvious from looking at ide-tape.c
(it is quite complicated code) - that is why I want to play it safe. ]
Thanks,
Bart
ditto
tape->restart_speed_control_req can still be non-zero.
Same as above: my main focus was ease of review and not keeping pipelining
functional at all times.
The review is _not_ easied by introducing new & not completely defined
states of the operation...
[ i.e. the speed control feedback loop should be either left alone or
removed altogether in some pre/post-patch instead of subtle changes
in the intermediate patches. ]
Could please recast the patches?
Thanks,
Bart
On Mon, Mar 03, 2008 at 12:16:29AM +0100, Bartlomiej Zolnierkiewicz wrote:
On Sunday 02 March 2008, Borislav Petkov wrote:
quoted
On Sun, Mar 02, 2008 at 07:33:05PM +0100, Bartlomiej Zolnierkiewicz wrote:
quoted
On Saturday 01 March 2008, Borislav Petkov wrote:
quoted
Instead of plugging the request into the pipeline, queue it straight on the
device's request queue through idetape_queue_rw_tail().
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 81 ++---------------------------------------------
1 files changed, 4 insertions(+), 77 deletions(-)
@@ -2123,12 +2123,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,debug_log(DBG_SENSE,"%s: cmd=%d\n",__func__,cmd);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-printk(KERN_ERR"ide-tape: bug: the pipeline is active in %s\n",-__func__);-return(0);-}-idetape_init_rq(&rq,cmd);rq.rq_disk=tape->disk;rq.special=(void*)bh;
@@ -2140,10 +2134,9 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,if((cmd&(REQ_IDETAPE_READ|REQ_IDETAPE_WRITE))==0)return0;-if(tape->merge_stage)-idetape_init_merge_stage(tape);if(rq.errors==IDETAPE_ERROR_GENERAL)return-EIO;+return(tape->blk_size*(blocks-rq.current_nr_sectors));}
These two changes to idetape_queue_rw_tail() don't look correct
as the pipeline mode is still used by read requests.
Wrt first hunk read rq pipeline functionality is removed in the following
patch. Would it then be better to merge the two patches? Actually, do we need
Merging patches together would cause increased complexity.
The best solution would be to move this hunk to the patch which removes
IDETAPE_FLAG_PIPELINE_ACTIVE flag.
quoted
to keep the driver functional in between the patches of those series for
the purposes of git bisection or only compile-testing it is enough? Cause
We want to keep the driver functional in between the patches, especially
given that it shouldn't be much more difficult to do so.
quoted
after applying the whole series you get pipelined mode ripped out anyway and
intermittent states with partially functional pipeline are of no importance, no?
We always want fully bisectable patches:
- if the patch series accidentaly completely breaks the code we want to be
able narrow it down to the guilty change
- if the patch series causes some regressions (ie. worse performance) we
also want to see which exact change caused it
[ Nothing changes here and removal of pipeline functionality can't be an
excuse for not sticking to the standard procedure. ]
Ok this changes the approach vector :). Will redo the patches from this angle
and send them in small b(i|y)tes :) in order to review them easier/faster.
Thanks.
--
Regards/Gruß,
Boris.
On Mon, Mar 03, 2008 at 12:16:29AM +0100, Bartlomiej Zolnierkiewicz wrote:
quoted
On Sunday 02 March 2008, Borislav Petkov wrote:
quoted
On Sun, Mar 02, 2008 at 07:33:05PM +0100, Bartlomiej Zolnierkiewicz wrote:
quoted
On Saturday 01 March 2008, Borislav Petkov wrote:
quoted
Instead of plugging the request into the pipeline, queue it straight on the
device's request queue through idetape_queue_rw_tail().
Signed-off-by: Borislav Petkov <redacted>
---
drivers/ide/ide-tape.c | 81 ++---------------------------------------------
1 files changed, 4 insertions(+), 77 deletions(-)
@@ -2123,12 +2123,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,debug_log(DBG_SENSE,"%s: cmd=%d\n",__func__,cmd);-if(test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,&tape->flags)){-printk(KERN_ERR"ide-tape: bug: the pipeline is active in %s\n",-__func__);-return(0);-}-idetape_init_rq(&rq,cmd);rq.rq_disk=tape->disk;rq.special=(void*)bh;
@@ -2140,10 +2134,9 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,if((cmd&(REQ_IDETAPE_READ|REQ_IDETAPE_WRITE))==0)return0;-if(tape->merge_stage)-idetape_init_merge_stage(tape);if(rq.errors==IDETAPE_ERROR_GENERAL)return-EIO;+return(tape->blk_size*(blocks-rq.current_nr_sectors));}
These two changes to idetape_queue_rw_tail() don't look correct
as the pipeline mode is still used by read requests.
Wrt first hunk read rq pipeline functionality is removed in the following
patch. Would it then be better to merge the two patches? Actually, do we need
Merging patches together would cause increased complexity.
The best solution would be to move this hunk to the patch which removes
IDETAPE_FLAG_PIPELINE_ACTIVE flag.
quoted
to keep the driver functional in between the patches of those series for
the purposes of git bisection or only compile-testing it is enough? Cause
We want to keep the driver functional in between the patches, especially
given that it shouldn't be much more difficult to do so.
quoted
after applying the whole series you get pipelined mode ripped out anyway and
intermittent states with partially functional pipeline are of no importance, no?
We always want fully bisectable patches:
- if the patch series accidentaly completely breaks the code we want to be
able narrow it down to the guilty change
- if the patch series causes some regressions (ie. worse performance) we
also want to see which exact change caused it
[ Nothing changes here and removal of pipeline functionality can't be an
excuse for not sticking to the standard procedure. ]
Ok this changes the approach vector :). Will redo the patches from this angle
and send them in small b(i|y)tes :) in order to review them easier/faster.
Thanks. I'll be waiting with review/merge for the re-done patch series then.
Bart
On Sat, Mar 01, 2008 at 12:20:38PM +0200, Adrian Bunk wrote:
quoted
On Sat, Mar 01, 2008 at 09:58:24AM +0100, Borislav Petkov wrote:
quoted
Hi Bart,
here's the 1st draft of the pipeline removal series. As the diffstat below openly
states it, a lot of code got removed - even more than the cleanup series we did
earlier.
...
After seeing commit d48567dd43868b3d2e1fcc33ee76dc2d38a1ddeb that
schedules ide-tape for removal I'm wondering whether any cleanups of
this driver make any sense at all:
Hi Adrian,
you're right and I'm expecting Bart's input on that. However, removing
tape support in ide altogether is probably not the right thing to do and
Bart wanted to keep some kind of a basic, ide-tape "light" version in so,
yes, the above commit is misleading.
Here's a fix, Bart please apply.
--
From 11c41d7760dd0b8f4cd1ab3076c86a2c4beec4de Mon Sep 17 00:00:00 2001
From: Borislav Petkov <redacted>
Date: Sat, 1 Mar 2008 16:31:17 +0100
Subject: [PATCH] ide-tape: keep a light version in ide tree
Keep a light version of the driver in for the
small amount of ide tape hardware still using it.
Signed-off-by: Borislav Petkov <redacted>
---
Documentation/feature-removal-schedule.txt | 10 ----------
1 files changed, 0 insertions(+), 10 deletions(-)
Since there was also an warning in the driver I reverted the original
commit instead.
From ca4e2ab5b2764562fe3d41b95b27e6bbd4733d66 Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Sat, 22 Mar 2008 16:44:27 +0100
Subject: [PATCH] Revert "ide-tape: schedule driver for removal after 6 months"
This reverts commit d48567dd43868b3d2e1fcc33ee76dc2d38a1ddeb.
Borislav is working on ide-tape "light" version instead.
Cc: Borislav Petkov <redacted>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
Documentation/feature-removal-schedule.txt | 10 ----------
drivers/ide/ide-tape.c | 5 -----
2 files changed, 0 insertions(+), 15 deletions(-)
@@ -172,16 +172,6 @@ Who: Len Brown <len.brown@intel.com> ----------------------------What: ide-tape driver-When: July 2008-Files: drivers/ide/ide-tape.c-Why: This driver might not have any users anymore and maintaining it for no- reason is an effort no one wants to make.-Who: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>, Borislav Petkov- <petkovbb@googlemail.com>------------------------------ What: libata spindown skipping and warning When: Dec 2008 Why: Some halt(8) implementations synchronize caches for and spin
@@ -3765,11 +3765,6 @@ static int ide_tape_probe(ide_drive_t *drive)g->fops=&idetape_block_ops;ide_register_region(g);-printk(KERN_WARNING"It is possible that this driver does not have any"-" users anymore and, as a result, it will be REMOVED soon."-" Please notify Bart <bzolnier@gmail.com> or Boris"-" <petkovbb@gmail.com> in case you still need it.\n");-return0;out_free_tape: