Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757330AbZCCQZm (ORCPT ); Tue, 3 Mar 2009 11:25:42 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753297AbZCCQZd (ORCPT ); Tue, 3 Mar 2009 11:25:33 -0500 Received: from accolon.hansenpartnership.com ([76.243.235.52]:34433 "EHLO accolon.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752920AbZCCQZb (ORCPT ); Tue, 3 Mar 2009 11:25:31 -0500 Subject: Re: Large amount of scsi-sgpool objects From: James Bottomley To: Jan Engelhardt Cc: Boaz Harrosh , linux-scsi@vger.kernel.org, Linux Kernel Mailing List , linux-rt-users@vger.kernel.org In-Reply-To: References: <49ACF8FE.2020904@panasas.com> <1236093718.3263.3.camel@localhost.localdomain> Content-Type: text/plain Date: Tue, 03 Mar 2009 16:25:26 +0000 Message-Id: <1236097526.3263.17.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 (2.22.3.1-1.fc9) Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6978 Lines: 188 On Tue, 2009-03-03 at 17:08 +0100, Jan Engelhardt wrote: > On Tuesday 2009-03-03 16:21, James Bottomley wrote: > >> > $ slabtop > >> > OBJS ACTIVE USE OBJ SIZE SLABS OBJ/SLAB CACHE SIZE NAME > >> > 818616 818616 100% 0.16K 34109 24 136436K sgpool-8 > >> > 253692 253692 100% 0.62K 42282 6 169128K sgpool-32 > >> > 52017 52016 99% 2.50K 17339 3 138712K sgpool-128 > >> > 26220 26219 99% 0.31K 2185 12 8740K sgpool-16 > >> > 8927 8574 96% 0.03K 79 113 316K size-32 > >> > >> Looks like a leak, by failing to call scsi_release_buffers() > >> somehow. (Which was changed recently) > > > >Firstly, I have to say I don't see this in the mainline tree, so could > >you try that with your setup just to verify (git head at 2.6.29-rc6). > > Yes, looking at the rt patch (in broken-out it's in origin.diff), > it seems a bit obvious - the scsi_release_buffers is not called anymore: OK, this is a bad patch, so just revert it. It was posted to linux-scsi initially in this form before the author posted a new one with the missing release buffers added. It looks like the first incarnation got pulled into the -rt tree for some reasons. So the real question is why does the -rt tree even have patches not in the vanilla SCSI tree? This type of cockup clearly demonstrates why it's a bad idea. James > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 940dc32..d4c6ac3 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -703,71 +703,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost) > > static void __scsi_release_buffers(struct scsi_cmnd *, int); > > -/* > - * Function: scsi_end_request() > - * > - * Purpose: Post-processing of completed commands (usually invoked at end > - * of upper level post-processing and scsi_io_completion). > - * > - * Arguments: cmd - command that is complete. > - * error - 0 if I/O indicates success, < 0 for I/O error. > - * bytes - number of bytes of completed I/O > - * requeue - indicates whether we should requeue leftovers. > - * > - * Lock status: Assumed that lock is not held upon entry. > - * > - * Returns: cmd if requeue required, NULL otherwise. > - * > - * Notes: This is called for block device requests in order to > - * mark some number of sectors as complete. > - * > - * We are guaranteeing that the request queue will be goosed > - * at some point during this call. > - * Notes: If cmd was requeued, upon return it will be a stale pointer. > - */ > -static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error, > - int bytes, int requeue) > -{ > - struct request_queue *q = cmd->device->request_queue; > - struct request *req = cmd->request; > - > - /* > - * If there are blocks left over at the end, set up the command > - * to queue the remainder of them. > - */ > - if (blk_end_request(req, error, bytes)) { > - int leftover = (req->hard_nr_sectors << 9); > - > - if (blk_pc_request(req)) > - leftover = req->data_len; > - > - /* kill remainder if no retrys */ > - if (error && scsi_noretry_cmd(cmd)) > - blk_end_request(req, error, leftover); > - else { > - if (requeue) { > - /* > - * Bleah. Leftovers again. Stick the > - * leftovers in the front of the > - * queue, and goose the queue again. > - */ > - scsi_release_buffers(cmd); > - scsi_requeue_command(q, cmd); > - cmd = NULL; > - } > - return cmd; > - } > - } > - > - /* > - * This will goose the queue request function at the end, so we don't > - * need to worry about launching another command. > - */ > - __scsi_release_buffers(cmd, 0); > - scsi_next_command(cmd); > - return NULL; > -} > - > static inline unsigned int scsi_sgtable_index(unsigned short nents) > { > unsigned int index; > @@ -929,7 +864,6 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd) > void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) > { > int result = cmd->result; > - int this_count; > struct request_queue *q = cmd->device->request_queue; > struct request *req = cmd->request; > int error = 0; > @@ -980,18 +914,30 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) > SCSI_LOG_HLCOMPLETE(1, printk("%ld sectors total, " > "%d bytes done.\n", > req->nr_sectors, good_bytes)); > - > - /* A number of bytes were successfully read. If there > - * are leftovers and there is some kind of error > - * (result != 0), retry the rest. > - */ > - if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL) > + if (blk_end_request(req, error, good_bytes) == 0) { > + /* This request is completely finished; start the next one */ > + scsi_next_command(cmd); > return; > - this_count = blk_rq_bytes(req); > + } > > error = -EIO; > > - if (host_byte(result) == DID_RESET) { > + /* The request isn't finished yet. Figure out what to do next. */ > + if (result == 0) { > + /* No error, so carry out the remainder of the request. > + * Failure to make forward progress counts against the > + * the number of retries. > + */ > + if (good_bytes > 0 || --req->retries >= 0) > + action = ACTION_REPREP; > + else { > + action = ACTION_FAIL; > + description = "Retries exhausted"; > + } > + } else if (error && scsi_noretry_cmd(cmd)) { > + /* Retrys are disallowed, so kill the remainder. */ > + action = ACTION_FAIL; > + } else if (host_byte(result) == DID_RESET) { > /* Third party bus reset or reset for error recovery > * reasons. Just retry the command and see what > * happens. > > > > Would you happen to know where I have to reinsert them in the > rt modification shown here? > > >If this holds true, there must be a bad patch in the -rt tree. You > >should be able to diff scsi_lib.c to see if there's something missing. > > > >Finally, there are one or two drivers (SCSI target) that do their own > >buffer management, so what drivers are you using? > > > ata1.00: ATA-5: WDC WD400EB-00CPF0, 06.04G06, max UDMA/100 > ata1.00: 78165360 sectors, multi 16: LBA > ata1.01: ATAPI: HL-DT-ST DVDRAM GSA-4160B, A301, max UDMA/66 > ata1.00: configured for UDMA/100 > ata1.01: configured for UDMA/66 > scsi 0:0:0:0: Direct-Access ATA WDC WD400EB-00CP 06.0 PQ: 0 ANSI: 5 > scsi 0:0:1:0: CD-ROM HL-DT-ST DVDRAM GSA-4160B A301 PQ: 0 ANSI: 5 > ata2.00: ATA-7: DIAMOND 250G 2B5400, RAMB1TU0, max UDMA/133 > ata2.00: 490234752 sectors, multi 16: LBA48 > ata2.00: configured for UDMA/133 > scsi 1:0:0:0: Direct-Access ATA DIAMOND 250G 2B RAMB PQ: 0 ANSI: 5 > > > > > Jan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/